Re: [PATCH 1/2] transport-helper: report errors properly
Felipe Contreras felipe.contre...@gmail.com writes: On Sat, Apr 13, 2013 at 1:00 AM, Jeff King p...@peff.net wrote: On Sat, Apr 13, 2013 at 12:42:29AM -0500, Felipe Contreras wrote: To me, the reality is obvious: my patch didn't require such a big commit message, the short version was fine, the only reason Jeff King insisted on a longer version is because the patch came from me. Get over yourself. The reason I suggested a longer commit message for your commit is because after spending several hours figuring out what the current code did, and what it should be doing instead, I wanted to document that effort so that I and other readers did not have to do it again later. ... The double standard might not come from you, perhaps you subject all the patches you review to the same standard, it comes from the fact that the patches you review have an unfair disadvantage. There are reviewers who share the basic values [*1*] with I and the tradition of this project and whose judgement I can trust. When somebody (like Peff) whose judgement I trust spends time to review a series, and writes his thought process to the degree that he thinks is appropriate, that's his judgement and I trust it. You cannot expect perfect evenness from multiple people. For that matter, you cannot expect perfect evenness even from a single person, either. When reviewing a proposed change to an area that I am intimately familiar with, I may immediately know some subtleties involved in a proposed solution without even reading the patch, and I may not even realize that such subtleties are hard to know without being somebody who are already familiar with that part of the codebase, and either in-code comment or in the log message may better spelled them out. On the other hand, when the change is in another area that I am not familiar with, I may request more explanation, if only for me to understand the issue. I try to avoid the I may know too well pitfalls, but I am not perfect. I will not speak for Peff or any other reviewers whose jugement I trust, but I would be very surprised if any of them claimed he is perfectly even. Double may only be showing that we do not have enough trusted maintainers; ideally I would like it to have Triple or more. [Footnote] *1* A few examples of core values. - we should make sure that future developers who wonder why a part of the code is how it is can find out what thought process brought the code into the current shape. - when we add something, we try not to overengineer or to shoot for unattainable perfection, but we still try to make sure we will not paint ourselves into an unescapable corner when we later want to extend 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 1/2] transport-helper: report errors properly
On Sun, Apr 14, 2013 at 12:23 AM, Junio C Hamano gits...@pobox.com wrote: Double may only be showing that we do not have enough trusted maintainers; ideally I would like it to have Triple or more. A double or triple review raises *a single* standard higher, but having more than one standard is never good. But apparently your trust in Jeff means more than caring about the double standard. But good to know, my patches will always have an unfair disadvantages to everybody else's. - when we add something, we try not to overengineer or to shoot for unattainable perfection, but we still try to make sure we will not paint ourselves into an unescapable corner when we later want to extend it. And there is such a thing as being too cautious, to the point where you walk WAY too slowly for the fear of tumbling, because it happened a few times in the fast, when you were running. But how would you even know that you are being too cautions? If you don't dare to walk a bit faster to find out. Cheers. -- 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 1/2] transport-helper: report errors properly
On Sat, Apr 13, 2013 at 12:42:29AM -0500, Felipe Contreras wrote: To me, the reality is obvious: my patch didn't require such a big commit message, the short version was fine, the only reason Jeff King insisted on a longer version is because the patch came from me. Get over yourself. The reason I suggested a longer commit message for your commit is because after spending several hours figuring out what the current code did, and what it should be doing instead, I wanted to document that effort so that I and other readers did not have to do it again later. I didn't even review the other patch you mention, so I could not possibly have come to the same point with it. But hey, if you want to have paranoid fantasies that I'm persecuting you (by writing the longer commit messages for you!), go ahead. If you don't want me to review your patches, that's fine by me, too; our discussions often end up frustrating, and it's clear we do not agree on very much with respect to process or design. But if you don't want that, please stop cc'ing me when you send out the 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 1/2] transport-helper: report errors properly
On Sat, Apr 13, 2013 at 1:00 AM, Jeff King p...@peff.net wrote: On Sat, Apr 13, 2013 at 12:42:29AM -0500, Felipe Contreras wrote: To me, the reality is obvious: my patch didn't require such a big commit message, the short version was fine, the only reason Jeff King insisted on a longer version is because the patch came from me. Get over yourself. The reason I suggested a longer commit message for your commit is because after spending several hours figuring out what the current code did, and what it should be doing instead, I wanted to document that effort so that I and other readers did not have to do it again later. I didn't even review the other patch you mention, so I could not possibly have come to the same point with it. The double standard might not come from you, perhaps you subject all the patches you review to the same standard, it comes from the fact that the patches you review have an unfair disadvantage. But hey, if you want to have paranoid fantasies that I'm persecuting you (by writing the longer commit messages for you!), go ahead. You don't persecute me, you persecute my patches. I could almost picture the moment you see a patch is coming from me, you have already decided to rewrite the commit message, even before reading it. Antoine is not me, so you simply didn't review that patch. If you don't want me to review your patches, that's fine by me, too; our discussions often end up frustrating, and it's clear we do not agree on very much with respect to process or design. But if you don't want that, please stop cc'ing me when you send out the patches. This comment was directed towards Junio, I do hope he is able to see the double standard. As for you, I think your reviews have value, but I also think you dwelling in irrelevant details do slow things down, which is not too bad, what is bad is that you assume that your opinions are facts (e.g. the commit message need to be bigger), and get angry when somebody disagrees with them. Cheers. -- 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 1/2] transport-helper: report errors properly
On Thu, Apr 11, 2013 at 6:05 PM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@gmail.com writes: And if you must, you might was well label them with REMINDER, no, wait, that's what TODO comments are for, where people can see them, and not *forget* them. Yeah, good point. Moreover, I think there's a clear double standard. Consider this commit: commit 99d3206010ba1fcc9311cbe8376c0b5e78f4a136 Author: Antoine Pelisse apeli...@gmail.com Date: Sat Mar 23 18:23:28 2013 +0100 combine-diff: coalesce lost lines optimally This replaces the greedy implementation to coalesce lost lines by using dynamic programming to find the Longest Common Subsequence. The O(n²) time complexity is obviously bigger than previous implementation but it can produce shorter diff results (and most likely easier to read). List of lost lines is now doubly-linked because we reverse-read it when reading the direction matrix. The commit message is 9 lines, and the diffstat 320 insertions(+), 64 deletions(-). Moreover, there are some important bits of information on the mailing list that never made it to the commit message: --- Best-case analysis: All p parents have the same n lines. We will find LCS and provide a n lines (the same lines) new list in O(n²), and then run it again in O(n²) with the next parent, etc. It will end-up being O(pn²). Worst-case analysis: All p parents have no lines in common. We will find LCS and provide a 2n new list in O(n²). Then we run it again in O(2n x n), and again O(3n x n), etc, until O(pn x n). When we sum these all, we end-up with O(p² x n²) --- --- Unfortunately on a commit that would remove A LOT of lines (1) from 7 parents, the times goes from 0.01s to 1.5s... I'm pretty sure that scenario is quite uncommon though. --- This is not mentioned in the commit message; on which situations this implementation would be worst and why it's OK either way. --- As you can see the last test is broken because the solution is not optimal for more than two parents. It would probably require to extend the dynamic programming to a k-dimension matrix (for k parents) but the result would end-up being O(n^k) (when removing n consecutives lines from p parents). I'm not sure there is any better solution known yet to the k-LCS problem. Implementing the dynamic solution with the k-dimension matrix would probably require to re-hash the strings (I guess it's already done by xdiff), as the number of string comparisons would increase. --- The fact that the last test is broken is not mentioned at all. Now let's compare to the final version of my patch which is 19 lines 40 insertions(+), 1 deletion(-). The ration of commit message lines vs. code changed lines is 19/41(0.46) whereas Antoine's patch is 3/128(0.02), a difference of over 19 times. Granted, some single-line changes do require a good chunk of explanation, but this is not one of them; this single line patch doesn't even change the behavior of the code, simply changes a silent error exit to a verbose error exit, that's all. Antoine's patch has a lot more potential to trigger something unexpected. And the chances that somebody would have to look at Antoine's patch is quite high, especially since a failing test-case is introduced. The chances that anybody would look at mine are very very low. So either Antoine's commit message was fine, and so was mine, or it was sorely lacking explanation. To me, the reality is obvious: my patch didn't require such a big commit message, the short version was fine, the only reason Jeff King insisted on a longer version is because the patch came from me. Antoine's patch might have benefited from a little more explanation, but not every issue that was discussed in the mailing list was necessary (in my patch virtually every issue discussed was added to the commit message). This is the definition of double standard. Cheers. -- 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 1/2] transport-helper: report errors properly
On Wed, Apr 10, 2013 at 4:15 PM, Jeff King p...@peff.net wrote: From: Felipe Contreras felipe.contre...@gmail.com If a push fails because the remote-helper died (with fast-export), the user does not see any error message. We do correctly die with a failed exit code, as we notice that the helper has died while reading back the ref status from the helper. However, we don't print any message. This is OK if the helper itself printed a useful error message, but we cannot count on that; let's let the user know that the helper failed. This explained the same thing: If a push fails because the remote-helper died (with fast-export), the user won't see any error message. So let's add one. Granted, depending on the way the remote-helper died, an error might or might not been printed, so s/won't/might not/. The fact that an exit code was returned before is not relevant, neither is how the exit was returned, and for that matter neither is all the other things that are happening in this code. It's just noise. The only thing that is relevant is this: - exit(128); + die(Reading from remote helper failed); It's a simple change, and simple to explain. In the long run, it may make more sense to propagate the error back up to push, so that it can present the usual status table and give a nicer message. But this is a much simpler fix that can help immediately. Yes it might, and it might make sense to rewrite much of this code, but that's not relevant. While we're adding tests, let's also confirm that the remote-helper dying is also detect when importing refs. That is enough explanation. We currently do so robustly when the helper uses the done feature (and that is what we test). We cannot do so reliably when the helper does not use the done feature, but it is not even worth testing; the right solution is for the helper to start using done. This doesn't help anyone, and it's not even accurate. I think it might be possible enforce remote-helpers to implement the done feature, and we might want to do that later. But of course, discussing what bad things remote-helpers could do, and how we should test and babysit them is not relevant here. If it was important to explain the subtleties and reasoning behind this change, it should be a separate patch. Suggested-by: Jeff King p...@peff.net Signed-off-by: Felipe Contreras felipe.contre...@gmail.com Signed-off-by: Jeff King p...@peff.net I would add: [jk: rewrote every piece of text] export) + if test -n $GIT_REMOTE_TESTGIT_FAILURE + then + # consume input so fast-export doesn't get SIGPIPE; I think this is explanation enough. + # git would also notice that case, but we want + # to make sure we are exercising the later + # error checks I don't understand what is being said here. What is that case? + while read line; do + test done = $line break + done + exit LGTM. Cheers. -- 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 1/2] transport-helper: report errors properly
On Thu, Apr 11, 2013 at 08:22:26AM -0500, Felipe Contreras wrote: We currently do so robustly when the helper uses the done feature (and that is what we test). We cannot do so reliably when the helper does not use the done feature, but it is not even worth testing; the right solution is for the helper to start using done. This doesn't help anyone, and it's not even accurate. I think it might be possible enforce remote-helpers to implement the done feature, and we might want to do that later. But of course, discussing what bad things remote-helpers could do, and how we should test and babysit them is not relevant here. If it was important to explain the subtleties and reasoning behind this change, it should be a separate patch. I am OK with adding the test for import as a separate patch. What I am not OK with (and this goes for the rest of the commit message, too) is failing to explain any back-story at all for why the change is done in the way it is. _You_ may understand it _right now_, but that is not the primary audience of the message. The primary audience is somebody else a year from now who is wondering why this patch was done the way it was. When they are trying to find out why git does not detect errors in a helper, and they notice that our test for failure only check the done case, isn't it more helpful to say we considered the other case, but it was not worth fixing rather than leaving them to guess? I may be more verbose than necessary in some of my commit messages, but I would much rather err on the side of explaining too much than too little. export) + if test -n $GIT_REMOTE_TESTGIT_FAILURE + then + # consume input so fast-export doesn't get SIGPIPE; I think this is explanation enough. + # git would also notice that case, but we want + # to make sure we are exercising the later + # error checks I don't understand what is being said here. What is that case? The case that fast-export gets SIGPIPE. I was trying to explain not just _what_ we are doing, but _why_ it is important. Perhaps a better wording would be: # consume input so fast-export doesn't get SIGPIPE; # we do not technically need to do so in order for # git to notice the failure to export, as it will # detect problems either with fast-export or with # the helper failing to report ref status. But since # we are trying to demonstrate that the latter # check works, we must avoid the SIGPIPE, which would # trigger the former. -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/2] transport-helper: report errors properly
On Thu, Apr 11, 2013 at 11:18 AM, Jeff King p...@peff.net wrote: On Thu, Apr 11, 2013 at 08:22:26AM -0500, Felipe Contreras wrote: We currently do so robustly when the helper uses the done feature (and that is what we test). We cannot do so reliably when the helper does not use the done feature, but it is not even worth testing; the right solution is for the helper to start using done. This doesn't help anyone, and it's not even accurate. I think it might be possible enforce remote-helpers to implement the done feature, and we might want to do that later. But of course, discussing what bad things remote-helpers could do, and how we should test and babysit them is not relevant here. If it was important to explain the subtleties and reasoning behind this change, it should be a separate patch. I am OK with adding the test for import as a separate patch. What I am not OK with (and this goes for the rest of the commit message, too) is failing to explain any back-story at all for why the change is done in the way it is. _You_ may understand it _right now_, but that is not the primary audience of the message. The primary audience is somebody else a year from now who is wondering why this patch was done the way it was. Who would be this person? Somebody who wonders why this test is using feature done? I doubt such a person would exist, as using this feature is standard, as can be seen below this chunk. *If* the test was *not* using this feature done, *then* sure, an explanation would be needed. But why is this test doing something expected is not a question anybody would benefit from asking. When they are trying to find out why git does not detect errors in a helper, and they notice that our test for failure only check the done case, isn't it more helpful to say we considered the other case, but it was not worth fixing rather than leaving them to guess? If you are worried about such hypothetical people, they would be better served by a comment in the source code of the test, or even better, the c file, or even better, to document that remote helpers should use this feature. But wait: --- Just like 'push', a batch sequence of one or more 'import' is terminated with a blank line. For each batch of 'import', the remote helper should produce a fast-import stream terminated by a 'done' command. --- So it's already explained, if somebody fails to follow this documentation, it's dubious a commit message that introduces a test would help. Surely, the writer of this bad remote helper would _never_ look there. I may be more verbose than necessary in some of my commit messages, but I would much rather err on the side of explaining too much than too little. I wouldn't. The only thing an overload of information achieves is that the reader would simply skip or skim it. export) + if test -n $GIT_REMOTE_TESTGIT_FAILURE + then + # consume input so fast-export doesn't get SIGPIPE; I think this is explanation enough. + # git would also notice that case, but we want + # to make sure we are exercising the later + # error checks I don't understand what is being said here. What is that case? The case that fast-export gets SIGPIPE. If we are trying to avoid SIGPIPE wouldn't that imply that git notices the SIGPIPE? # consume input so fast-export doesn't get SIGPIPE; # we do not technically need to do so in order for # git to notice the failure to export, as it will # detect problems either with fast-export or with # the helper failing to report ref status. But since # we are trying to demonstrate that the latter # check works, we must avoid the SIGPIPE, which would # trigger the former. # consume input so fast-export doesn't get SIGPIPE; we want to test the remote-helper's code after fast-export. -- 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 1/2] transport-helper: report errors properly
On Thu, Apr 11, 2013 at 11:49:11AM -0500, Felipe Contreras wrote: I am OK with adding the test for import as a separate patch. What I am not OK with (and this goes for the rest of the commit message, too) is failing to explain any back-story at all for why the change is done in the way it is. _You_ may understand it _right now_, but that is not the primary audience of the message. The primary audience is somebody else a year from now who is wondering why this patch was done the way it was. Who would be this person? Somebody who wonders why this test is using feature done? I doubt such a person would exist, as using this feature is standard, as can be seen below this chunk. *If* the test was *not* using this feature done, *then* sure, an explanation would be needed. If it was so obvious, why did your initial patch not use feature done? If it was so obvious, why did our email discussion go back and forth so many times before arriving at this patch? It was certainly not obvious to me when this email thread started. So in response to your question: *I* am that person. I was him two weeks ago, and there is a good chance that I will be him a year from now. Much of my work on git is spent tracking down bugs in older code, and those commit messages are extremely valuable to me in understanding what happened at the time. But I give up on you. I find most of your commit messages lacking in details and motivation, making assumptions that the reader is as familiar with the code when reading the commit as you are when you wrote it. I tried to help by suggesting in review that you elaborate. That didn't work. So I tried to help by writing the text myself. But clearly I am not going to convince you that it is valuable, even if it requires no work at all from you, so I have nothing else to say on the matter. -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/2] transport-helper: report errors properly
On Thu, Apr 11, 2013 at 11:59 AM, Jeff King p...@peff.net wrote: On Thu, Apr 11, 2013 at 11:49:11AM -0500, Felipe Contreras wrote: I am OK with adding the test for import as a separate patch. What I am not OK with (and this goes for the rest of the commit message, too) is failing to explain any back-story at all for why the change is done in the way it is. _You_ may understand it _right now_, but that is not the primary audience of the message. The primary audience is somebody else a year from now who is wondering why this patch was done the way it was. Who would be this person? Somebody who wonders why this test is using feature done? I doubt such a person would exist, as using this feature is standard, as can be seen below this chunk. *If* the test was *not* using this feature done, *then* sure, an explanation would be needed. If it was so obvious, why did your initial patch not use feature done? Because I didn't want to test the obvious, I wanted to test something else. If it was so obvious, why did our email discussion go back and forth so many times before arriving at this patch? This patch has absolutely nothing to do with that, in fact, forget about it, such a minor check is not worth this time and effort: http://article.gmane.org/gmane.comp.version-control.git/220899 It was certainly not obvious to me when this email thread started. So in response to your question: *I* am that person. I was him two weeks ago, and there is a good chance that I will be him a year from now. No, you are not. I didn't send a patch with feature done originally, the only reason you wondered about the patch with feature done is that you saw one without it. It will _never_ happen again. Much of my work on git is spent tracking down bugs in older code, and those commit messages are extremely valuable to me in understanding what happened at the time. Lets make a bet. Let's push the simpler version, and when you hit this commit message retrospectively and find that you don't understand what is happening, I loose, and I will forever accept verbose commit messages. It will never happen. But I give up on you. I find most of your commit messages lacking in details and motivation, making assumptions that the reader is as familiar with the code when reading the commit as you are when you wrote it. I tried to help by suggesting in review that you elaborate. That didn't work. So I tried to help by writing the text myself. But clearly I am not going to convince you that it is valuable, even if it requires no work at all from you, so I have nothing else to say on the matter. Me neither. I picked your solution, but that's not enough, you *always* want me to do EXACTLY what you want, and never argue back. It's not going to happen. There's nothing wrong with disagreeing. Cheers. -- 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 1/2] transport-helper: report errors properly
Felipe Contreras felipe.contre...@gmail.com writes: On Wed, Apr 10, 2013 at 4:15 PM, Jeff King p...@peff.net wrote: From: Felipe Contreras felipe.contre...@gmail.com If a push fails because the remote-helper died (with fast-export), the user does not see any error message. We do I agree with you that s/does not see/may not see/ would be more helpful here, so I'll squash it in while queuing. In the long run, it may make more sense to propagate the error back up to push, so that it can present the usual status table and give a nicer message. But this is a much simpler fix that can help immediately. Yes it might, and it might make sense to rewrite much of this code, but that's not relevant. It is a good reminder for people who later inspect this part of the code and wonder if it was a conscious design choice not to propagate the error or just being simple and sufficient for now, I think. It would help them by making it clear that it is the latter, no? ... I think it might be possible enforce remote-helpers to implement the done feature, and we might want to do that later. Yes, all these are possible and I think writing it down explicitly will serve as a reminder for our future selves, I think. + if test -n $GIT_REMOTE_TESTGIT_FAILURE + then + # consume input so fast-export doesn't get SIGPIPE; I think this is explanation enough. + # git would also notice that case, but we want + # to make sure we are exercising the later + # error checks I don't understand what is being said here. What is that case? In my first reading, it felt to me that it was natural to interpret that this is even if we didn't have this loop that avoids killing fast-export with SIGPIPE, we would notice death of fast-export by SIGPIPE. -- 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] transport-helper: report errors properly
Felipe Contreras felipe.contre...@gmail.com writes: On Thu, Apr 11, 2013 at 11:59 AM, Jeff King p...@peff.net wrote: But I give up on you. I find most of your commit messages lacking in details and motivation, making assumptions that the reader is as familiar with the code when reading the commit as you are when you wrote it. I tried to help by suggesting in review that you elaborate. That didn't work. So I tried to help by writing the text myself. But clearly I am not going to convince you that it is valuable, even if it requires no work at all from you, so I have nothing else to say on the matter. Me neither. I picked your solution, but that's not enough, you *always* want me to do EXACTLY what you want, and never argue back. It's not going to happen. There's nothing wrong with disagreeing. Heh, it seems that I was late for the party. Writing only minimally sufficient in the log messages is fine for your own project. We won't decide nor dictate the policy for your project for you. But _this_ project wants its log messages to be understandable by people who you may disagree with and who may have shorter memory span than you do. Disagreeing with that policy is fine. You need to learn to disagree but accept to be part of the project. 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 1/2] transport-helper: report errors properly
On Thu, Apr 11, 2013 at 1:44 PM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@gmail.com writes: In the long run, it may make more sense to propagate the error back up to push, so that it can present the usual status table and give a nicer message. But this is a much simpler fix that can help immediately. Yes it might, and it might make sense to rewrite much of this code, but that's not relevant. It is a good reminder for people who later inspect this part of the code and wonder if it was a conscious design choice not to propagate the error or just being simple and sufficient for now, I think. It would help them by making it clear that it is the latter, no? No. Design choices is what code comments are for, of which Git only has too few, according to ohloh[1]. No wonder they are so few, people are spending time writing novels on commit messages and forgetting there's also code where you should clarify things. ... I think it might be possible enforce remote-helpers to implement the done feature, and we might want to do that later. Yes, all these are possible and I think writing it down explicitly will serve as a reminder for our future selves, I think. Yes, but not writing them here. By spending so much time in commit messages you neglect the code, and the wiki (which is actually the place to write these things on. And if all you want is to write them down, we already did, right here. There's no need to punish the readers of the commit messages in the future only so we can flex our memory, because we already did. And if you must, you might was well label them with REMINDER, no, wait, that's what TODO comments are for, where people can see them, and not *forget* them. Cheers. [1] https://www.ohloh.net/p/git/factoids#FactoidCommentsLow -- 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 1/2] transport-helper: report errors properly
On Thu, Apr 11, 2013 at 1:49 PM, Junio C Hamano gits...@pobox.com wrote: Felipe Contreras felipe.contre...@gmail.com writes: On Thu, Apr 11, 2013 at 11:59 AM, Jeff King p...@peff.net wrote: But I give up on you. I find most of your commit messages lacking in details and motivation, making assumptions that the reader is as familiar with the code when reading the commit as you are when you wrote it. I tried to help by suggesting in review that you elaborate. That didn't work. So I tried to help by writing the text myself. But clearly I am not going to convince you that it is valuable, even if it requires no work at all from you, so I have nothing else to say on the matter. Me neither. I picked your solution, but that's not enough, you *always* want me to do EXACTLY what you want, and never argue back. It's not going to happen. There's nothing wrong with disagreeing. Heh, it seems that I was late for the party. Writing only minimally sufficient in the log messages is fine for your own project. We won't decide nor dictate the policy for your project for you. But _this_ project wants its log messages to be understandable by people who you may disagree with and who may have shorter memory span than you do. Having a shorter memory span is irrelevant when you are _never_ going to go back and ask the question the commit message is answering. And if it indeed is an important question, the answer belongs in the code comments. Disagreeing with that policy is fine. You need to learn to disagree but accept to be part of the project. Yeah, I accept that you will commit whatever you want, but I still don't think this verbosity serves the purpose you think it serves. Some one-liners deserve pages of commit messages, but this one is not one of them. People are easily deceived, and because you saw one commit message that needed more information, you think all of them do, but no, some don't don't, and this is one of them. It's not serving any real purpose. -- 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 1/2] transport-helper: report errors properly
Felipe Contreras felipe.contre...@gmail.com writes: And if you must, you might was well label them with REMINDER, no, wait, that's what TODO comments are for, where people can see them, and not *forget* them. Yeah, good point. -- 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/2] transport-helper: report errors properly
From: Felipe Contreras felipe.contre...@gmail.com
If a push fails because the remote-helper died (with
fast-export), the user does not see any error message. We do
correctly die with a failed exit code, as we notice that the
helper has died while reading back the ref status from the
helper. However, we don't print any message. This is OK if
the helper itself printed a useful error message, but we
cannot count on that; let's let the user know that the
helper failed.
In the long run, it may make more sense to propagate the
error back up to push, so that it can present the usual
status table and give a nicer message. But this is a much
simpler fix that can help immediately.
While we're adding tests, let's also confirm that the
remote-helper dying is also detect when importing refs. We
currently do so robustly when the helper uses the done
feature (and that is what we test). We cannot do so
reliably when the helper does not use the done feature,
but it is not even worth testing; the right solution is for
the helper to start using done.
Suggested-by: Jeff King p...@peff.net
Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
Signed-off-by: Jeff King p...@peff.net
---
Felipe,
Can you acknowledge that it's OK to stick your name on this, as it's not
exactly what you submitted before?
git-remote-testgit| 19 +++
t/t5801-remote-helpers.sh | 20
transport-helper.c| 2 +-
3 files changed, 40 insertions(+), 1 deletion(-)
diff --git a/git-remote-testgit b/git-remote-testgit
index b395c8d..5fd09f9 100755
--- a/git-remote-testgit
+++ b/git-remote-testgit
@@ -61,12 +61,31 @@ do
echo feature import-marks=$gitmarks
echo feature export-marks=$gitmarks
fi
+
+ if test -n $GIT_REMOTE_TESTGIT_FAILURE
+ then
+ echo feature done
+ exit 1
+ fi
+
echo feature done
git fast-export ${testgitmarks_args[@]} $refs |
sed -e s#refs/heads/#${prefix}/heads/#g
echo done
;;
export)
+ if test -n $GIT_REMOTE_TESTGIT_FAILURE
+ then
+ # consume input so fast-export doesn't get SIGPIPE;
+ # git would also notice that case, but we want
+ # to make sure we are exercising the later
+ # error checks
+ while read line; do
+ test done = $line break
+ done
+ exit 1
+ fi
+
before=$(git for-each-ref --format='%(refname) %(objectname)')
git fast-import ${testgitmarks_args[@]} --quiet
diff --git a/t/t5801-remote-helpers.sh b/t/t5801-remote-helpers.sh
index f387027..aafc46a 100755
--- a/t/t5801-remote-helpers.sh
+++ b/t/t5801-remote-helpers.sh
@@ -166,4 +166,24 @@ test_expect_success 'push ref with existing object' '
compare_refs local dup server dup
'
+test_expect_success 'proper failure checks for fetching' '
+ (GIT_REMOTE_TESTGIT_FAILURE=1
+ export GIT_REMOTE_TESTGIT_FAILURE
+ cd local
+ test_must_fail git fetch 2 error
+ cat error
+ grep -q Error while running fast-import error
+ )
+'
+
+test_expect_success 'proper failure checks for pushing' '
+ (GIT_REMOTE_TESTGIT_FAILURE=1
+ export GIT_REMOTE_TESTGIT_FAILURE
+ cd local
+ test_must_fail git push --all 2 error
+ cat error
+ grep -q Reading from remote helper failed error
+ )
+'
+
test_done
diff --git a/transport-helper.c b/transport-helper.c
index cb3ef7d..96081cc 100644
--- a/transport-helper.c
+++ b/transport-helper.c
@@ -54,7 +54,7 @@ static int recvline_fh(FILE *helper, struct strbuf *buffer)
if (strbuf_getline(buffer, helper, '\n') == EOF) {
if (debug)
fprintf(stderr, Debug: Remote helper quit.\n);
- exit(128);
+ die(Reading from remote helper failed);
}
if (debug)
--
1.8.2.rc0.33.gd915649
--
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] transport-helper: report errors properly
On Wed, Apr 10, 2013 at 2:15 PM, Jeff King p...@peff.net wrote: From: Felipe Contreras felipe.contre...@gmail.com If a push fails because the remote-helper died (with fast-export), the user does not see any error message. We do correctly die with a failed exit code, as we notice that the helper has died while reading back the ref status from the helper. However, we don't print any message. This is OK if the helper itself printed a useful error message, but we cannot count on that; let's let the user know that the helper failed. In the long run, it may make more sense to propagate the error back up to push, so that it can present the usual status table and give a nicer message. But this is a much simpler fix that can help immediately. While we're adding tests, let's also confirm that the remote-helper dying is also detect when importing refs. We currently do so robustly when the helper uses the done feature (and that is what we test). We cannot do so reliably when the helper does not use the done feature, but it is not even worth testing; the right solution is for the helper to start using done. Suggested-by: Jeff King p...@peff.net Signed-off-by: Felipe Contreras felipe.contre...@gmail.com Signed-off-by: Jeff King p...@peff.net The fixes you made to this patch make a lot of sense, glad to not have a 'sleep 1' in our tests. Acked-by: Sverre Rabbelier srabbel...@gmail.com -- Cheers, Sverre Rabbelier -- 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] transport-helper: report errors properly
On Wed, Apr 10, 2013 at 5:15 PM, Jeff King p...@peff.net wrote: From: Felipe Contreras felipe.contre...@gmail.com If a push fails because the remote-helper died (with fast-export), the user does not see any error message. We do correctly die with a failed exit code, as we notice that the helper has died while reading back the ref status from the helper. However, we don't print any message. This is OK if the helper itself printed a useful error message, but we cannot count on that; let's let the user know that the helper failed. In the long run, it may make more sense to propagate the error back up to push, so that it can present the usual status table and give a nicer message. But this is a much simpler fix that can help immediately. While we're adding tests, let's also confirm that the remote-helper dying is also detect when importing refs. We s/detect/detected/ currently do so robustly when the helper uses the done feature (and that is what we test). We cannot do so reliably when the helper does not use the done feature, but it is not even worth testing; the right solution is for the helper to start using done. Suggested-by: Jeff King p...@peff.net Signed-off-by: Felipe Contreras felipe.contre...@gmail.com Signed-off-by: Jeff King p...@peff.net -- 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
