Re: [PATCH 3/5] Fix hiding a message while some citations are shown in notmuch-show view.
On Fri, 27 May 2011 01:42:22 +0400, Dmitry Kurochkin dmitry.kuroch...@gmail.com wrote: I have just send two more patches to this thread. One with new tests. Another with a workaround for the bug. The workaround should not break anything since it affects only notmuch-show mode. Thanks! I love committing tests that demonstrate broken code before committing fixes. As it happened here, I committed these two new patches thinking I had previously committed the earlier patches in the series. Fortunately, the failure of the test pointed out that I was missing the actual fix. I also really like the workaround to avoid regressing functionality because of an emacs bug. Well done, Dmitry. I've now pushed out everything in this series. -Carl -- carl.d.wo...@intel.com pgpJBJjGfTyfI.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 3/5] Fix hiding a message while some citations are shown in notmuch-show view.
On Wed, 15 Jun 2011 07:06:40 -0700, Carl Worth cwo...@cworth.org wrote: On Fri, 27 May 2011 01:42:22 +0400, Dmitry Kurochkin dmitry.kuroch...@gmail.com wrote: I have just send two more patches to this thread. One with new tests. Another with a workaround for the bug. The workaround should not break anything since it affects only notmuch-show mode. Thanks! I love committing tests that demonstrate broken code before committing fixes. As it happened here, I committed these two new patches thinking I had previously committed the earlier patches in the series. Fortunately, the failure of the test pointed out that I was missing the actual fix. I know you prefer tests to go before patches and I agree with that. But most of the time I do tests after coding. I do not know an easy way to reorder patches in git. (Also I do not know how to amend an old patch, wish more darcs features in git.) Hopefully it is not a big trouble for you to reorder the patches when applying. I also really like the workaround to avoid regressing functionality because of an emacs bug. indeed Well done, Dmitry. I've now pushed out everything in this series. Thanks. Regards, Dmitry -Carl -- carl.d.wo...@intel.com ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 3/5] Fix hiding a message while some citations are shown in notmuch-show view.
On Wed, 15 Jun 2011 18:25:14 +0400, Dmitry Kurochkin dmitry.kuroch...@gmail.com wrote: I know you prefer tests to go before patches and I agree with that. But most of the time I do tests after coding. I do not know an easy way to reorder patches in git. (Also I do not know how to amend an old patch, wish more darcs features in git.) Hopefully it is not a big trouble for you to reorder the patches when applying. Hey, Dmitry. I usually just pop in to a new branch rooted before my new patch series, and then cherry-pick the new patches onto the branch in the order I want (eg. tests first, etc.). jamie. pgpaAiRVF0L52.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 3/5] Fix hiding a message while some citations are shown in notmuch-show view.
On Wed, 15 Jun 2011 18:25:14 +0400, Dmitry Kurochkin dmitry.kuroch...@gmail.com wrote: I know you prefer tests to go before patches and I agree with that. Great! But most of the time I do tests after coding. Yes, I do that order almost exclusively as well. I do not know an easy way to reorder patches in git. (Also I do not know how to amend an old patch Fortunately, git has a great feature here for both use cases, (git rebase -i). Here's the simple recipe: * Find a bug, fix a bug, commit * Write a test case, commit * Run the following command: git rebase -i origin/master At this point you'll be presented with an editor window giving one line for each commit that you have made since origin/master. You can reorder these lines however you'd like. When you save and exit the editor, the commits will be applied in the order you saved. If there are any conflicts due to the re-ordering, then git rebase will stop and tell you what to do, which will be: * Resolve the conflict * Run git add on the files you edited * Run git rebase --continue Also, back when editing the original list of commits, you can change the word apply next to any particular commit to change what happens when applying it. If you change that to reword you'll be given an editor window to edit the commit message. If you use edit then you'll be dropped to a shell where you can: * Edit the code * Test as necessary * Run git commit --amend * Run git rebase --continue I absolutely love git rebase -i. It's one of my favorite user-interface features in git. wish more darcs features in git. I don't know about git rebase -i, but I think I heard that git add -i, (interactively add some portions of the dirty working tree to the index to be committed). I think the menu-based interface of git add -i is particularly clunky. But I love the trimmed-down interface of git add -p which simply prompts one-patch-hunk-at-a-time for pieces to add to the next commit. It even supports splitting a hunk, (or even manually editing the patch to trim it down!). It's pretty slick stuff. So there are some git tips that might be useful. Thanks. You're quite welcome. Thanks for all the great work. Please keep it up! -Carl -- carl.d.wo...@intel.com pgpbqJ9DsetYl.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 3/5] Fix hiding a message while some citations are shown in notmuch-show view.
On Wed, 15 Jun 2011 10:00:36 -0700, Carl Worth cwo...@cworth.org wrote: On Wed, 15 Jun 2011 18:25:14 +0400, Dmitry Kurochkin dmitry.kuroch...@gmail.com wrote: I know you prefer tests to go before patches and I agree with that. Great! But most of the time I do tests after coding. Yes, I do that order almost exclusively as well. I do not know an easy way to reorder patches in git. (Also I do not know how to amend an old patch Fortunately, git has a great feature here for both use cases, (git rebase -i). Here's the simple recipe: * Find a bug, fix a bug, commit * Write a test case, commit * Run the following command: git rebase -i origin/master At this point you'll be presented with an editor window giving one line for each commit that you have made since origin/master. You can reorder these lines however you'd like. When you save and exit the editor, the commits will be applied in the order you saved. If there are any conflicts due to the re-ordering, then git rebase will stop and tell you what to do, which will be: * Resolve the conflict * Run git add on the files you edited * Run git rebase --continue Also, back when editing the original list of commits, you can change the word apply next to any particular commit to change what happens when applying it. If you change that to reword you'll be given an editor window to edit the commit message. If you use edit then you'll be dropped to a shell where you can: * Edit the code * Test as necessary * Run git commit --amend * Run git rebase --continue I absolutely love git rebase -i. It's one of my favorite user-interface features in git. Thanks for this. I did not know about interactive mode in rebase. This is some sort of replacement for darcs amend (which allows editing any patch, not just the last one). wish more darcs features in git. I don't know about git rebase -i, but I think I heard that git add -i, (interactively add some portions of the dirty working tree to the index to be committed). I think the menu-based interface of git add -i is particularly clunky. But I love the trimmed-down interface of git add -p which simply prompts one-patch-hunk-at-a-time for pieces to add to the next commit. It even supports splitting a hunk, (or even manually editing the patch to trim it down!). It's pretty slick stuff. Yes, add -i is ugl... confusing, but add -p is very nice. A great feature of darcs picked up by git. So there are some git tips that might be useful. They will be useful indeed. Thanks! Regards, Dmitry Thanks. You're quite welcome. Thanks for all the great work. Please keep it up! -Carl -- carl.d.wo...@intel.com ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 3/5] Fix hiding a message while some citations are shown in notmuch-show view.
On Wed, 25 May 2011 18:02:49 -0700, Carl Worth cwo...@cworth.org wrote: On Thu, 26 May 2011 03:10:11 +0400, Dmitry Kurochkin dmitry.kuroch...@gmail.com wrote: On Wed, 25 May 2011 15:46:40 -0700, Carl Worth cwo...@cworth.org wrote: Well, emacs trunk is not broken :) The bug is in lisp code, so you can fix it in .emacs by redefining `isearch-range-invisible' function. I do that now. Oh, in that case we can fix this is notmuch emacs lisp by just defining and using a fixed function. Is the broken function something we're calling directly? Or is it being called indirectly? (being called by other emacs lisp code that we are calling)? It is called indirectly. What is the best way to fix it? I imagine that we can replace `isearch-range-invisible' function with another one, which would call the fixed function if some special variable is set, or if we are searching in a notmuch-show view. Otherwise it would call the original function. If we can incorporate the fix, that would be great. Please consider pushing other patches from the series. They do not fix any bug, but do simplify the code. The last patch uses list for invisible overlay property as well. But it does not break isearch because we do not search in hidden messages. Hmmm... we should probably do that. I'd like isearch in notmuch to search anything that is hidden. That is easy to fix. I can do that once we are done with this stuff. BTW would be nice to have a set of known-to-fail tests, i.e. bugs that are not fixed yet. If we had it, the above test could be implemented and committed before we have the fix pushed. We do! Use test_expect_equal_failure (yes, the name is horrible!) instead of test_expect_equal and you should get what you want. I should look at it, thanks. Regards, Dmitry -Carl -- carl.d.wo...@intel.com ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 3/5] Fix hiding a message while some citations are shown in notmuch-show view.
On Thu, 26 May 2011 14:26:34 +0400, Dmitry Kurochkin dmitry.kuroch...@gmail.com wrote: It is called indirectly. What is the best way to fix it? I imagine that we can replace `isearch-range-invisible' function with another one, which would call the fixed function if some special variable is set, or if we are searching in a notmuch-show view. Otherwise it would call the original function. I'm not sure what the best approach would be. I'd like to be a good emacs citizen but I don't know what the best way to do that is here. If we can predict what the first emacs release will be that contains the fix, then we could restrict our kludge here to older, unfixed emacs. That would help avoid some problems with bad citizenship here. I'm willing to take whatever you this is best here. -Carl -- carl.d.wo...@intel.com pgpJoldixvzPn.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 3/5] Fix hiding a message while some citations are shown in notmuch-show view.
On Thu, 26 May 2011 14:31:30 -0700, Carl Worth cwo...@cworth.org wrote: On Thu, 26 May 2011 14:26:34 +0400, Dmitry Kurochkin dmitry.kuroch...@gmail.com wrote: It is called indirectly. What is the best way to fix it? I imagine that we can replace `isearch-range-invisible' function with another one, which would call the fixed function if some special variable is set, or if we are searching in a notmuch-show view. Otherwise it would call the original function. I'm not sure what the best approach would be. I'd like to be a good emacs citizen but I don't know what the best way to do that is here. Agreed on being a good emacs citizen. If we can predict what the first emacs release will be that contains the fix, then we could restrict our kludge here to older, unfixed emacs. That would help avoid some problems with bad citizenship here. I'm willing to take whatever you this is best here. I have just send two more patches to this thread. One with new tests. Another with a workaround for the bug. The workaround should not break anything since it affects only notmuch-show mode. Regards, Dmitry -Carl -- carl.d.wo...@intel.com Non-text part: application/pgp-signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 3/5] Fix hiding a message while some citations are shown in notmuch-show view.
On Thu, 26 May 2011 02:10:14 +0400, Dmitry Kurochkin dmitry.kuroch...@gmail.com wrote: Before the change, message and citation invisibility overlays conflicted: if some citation is made visible and then the whole message is hidden, that citation remained visible. That sounds like quite a bug. I'd love to see this series also add a test case for that. -Carl -- carl.d.wo...@intel.com pgpZT6IzLLblE.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 3/5] Fix hiding a message while some citations are shown in notmuch-show view.
On Wed, 25 May 2011 15:23:47 -0700, Carl Worth cwo...@cworth.org wrote: On Thu, 26 May 2011 02:10:14 +0400, Dmitry Kurochkin dmitry.kuroch...@gmail.com wrote: Before the change, message and citation invisibility overlays conflicted: if some citation is made visible and then the whole message is hidden, that citation remained visible. That sounds like quite a bug. I'd love to see this series also add a test case for that. I am not sure how it is best to test this. The common `printc' method for emacs tests does not work, because it prints invisible parts as well. We need either to find a way to print only visible text on the console, or test it inside emacs somehow. Any suggestions? Note that this is exactly the patch that hits the isearch emacs bug. Do I understand correctly that you are ready to push the series despite of it (given that we have a test)? Regards, Dmitry -Carl -- carl.d.wo...@intel.com Non-text part: application/pgp-signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 3/5] Fix hiding a message while some citations are shown in notmuch-show view.
On Thu, 26 May 2011 02:34:28 +0400, Dmitry Kurochkin dmitry.kuroch...@gmail.com wrote: I am not sure how it is best to test this. The common `printc' method for emacs tests does not work, because it prints invisible parts as well. We need either to find a way to print only visible text on the console, or test it inside emacs somehow. Any suggestions? Unfortunately, I don't have a good plan here. I delayed implementing any automated testing at all of the emacs interface precisely because of this problem. It's seems to me that surely emacs must have some built-in mechanism for copying the visible portion of a block of text, but I've not been able to find it. We could do something cheesy (and slow) by marching through the buffer character-by-character in elisp and testing for visibility, but the emacs tests are already the slowest part of make test[*] so that would be obnoxious. Note that this is exactly the patch that hits the isearch emacs bug. Do I understand correctly that you are ready to push the series despite of it (given that we have a test)? Breaking isearch would be really unfortunate. That's a really nice feature of the emacs frontend currently. So I would notice that breakage, (while I've apparently never before noticed the breakage of having visible citations in a hidden message). So no, I'm not saying I'm ready to push the series while emacs is broken. -Carl [*] Maybe the performance of the emacs testing could be significantly improved by sharing a single invocation of emacs? Perhaps this wouldn't even be hard by just using emacsclient? pgp6iFku77j2A.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 3/5] Fix hiding a message while some citations are shown in notmuch-show view.
On Wed, 25 May 2011 15:46:40 -0700, Carl Worth cwo...@cworth.org wrote: On Thu, 26 May 2011 02:34:28 +0400, Dmitry Kurochkin dmitry.kuroch...@gmail.com wrote: I am not sure how it is best to test this. The common `printc' method for emacs tests does not work, because it prints invisible parts as well. We need either to find a way to print only visible text on the console, or test it inside emacs somehow. Any suggestions? Unfortunately, I don't have a good plan here. I delayed implementing any automated testing at all of the emacs interface precisely because of this problem. It's seems to me that surely emacs must have some built-in mechanism for copying the visible portion of a block of text, but I've not been able to find it. Me too. We could do something cheesy (and slow) by marching through the buffer character-by-character in elisp and testing for visibility, but the emacs tests are already the slowest part of make test[*] so that would be obnoxious. Indeed. Note that this is exactly the patch that hits the isearch emacs bug. Do I understand correctly that you are ready to push the series despite of it (given that we have a test)? Breaking isearch would be really unfortunate. That's a really nice feature of the emacs frontend currently. So I would notice that breakage, (while I've apparently never before noticed the breakage of having visible citations in a hidden message). So no, I'm not saying I'm ready to push the series while emacs is broken. Well, emacs trunk is not broken :) The bug is in lisp code, so you can fix it in .emacs by redefining `isearch-range-invisible' function. I do that now. I do not think I will make myself work on the test until it is likely to be pushed. I will try to not to forget about it, so sometime later I may be back to it :) Please consider pushing other patches from the series. They do not fix any bug, but do simplify the code. The last patch uses list for invisible overlay property as well. But it does not break isearch because we do not search in hidden messages. BTW would be nice to have a set of known-to-fail tests, i.e. bugs that are not fixed yet. If we had it, the above test could be implemented and committed before we have the fix pushed. -Carl [*] Maybe the performance of the emacs testing could be significantly improved by sharing a single invocation of emacs? Perhaps this wouldn't even be hard by just using emacsclient? This is possible as long as tests do not affect each other. Would be a nice improvement. Regards, Dmitry ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
Re: [PATCH 3/5] Fix hiding a message while some citations are shown in notmuch-show view.
On Thu, 26 May 2011 03:10:11 +0400, Dmitry Kurochkin dmitry.kuroch...@gmail.com wrote: On Wed, 25 May 2011 15:46:40 -0700, Carl Worth cwo...@cworth.org wrote: Well, emacs trunk is not broken :) The bug is in lisp code, so you can fix it in .emacs by redefining `isearch-range-invisible' function. I do that now. Oh, in that case we can fix this is notmuch emacs lisp by just defining and using a fixed function. Is the broken function something we're calling directly? Or is it being called indirectly? (being called by other emacs lisp code that we are calling)? If we can incorporate the fix, that would be great. Please consider pushing other patches from the series. They do not fix any bug, but do simplify the code. The last patch uses list for invisible overlay property as well. But it does not break isearch because we do not search in hidden messages. Hmmm... we should probably do that. I'd like isearch in notmuch to search anything that is hidden. BTW would be nice to have a set of known-to-fail tests, i.e. bugs that are not fixed yet. If we had it, the above test could be implemented and committed before we have the fix pushed. We do! Use test_expect_equal_failure (yes, the name is horrible!) instead of test_expect_equal and you should get what you want. -Carl -- carl.d.wo...@intel.com pgp1vAI5Cj5J5.pgp Description: PGP signature ___ notmuch mailing list notmuch@notmuchmail.org http://notmuchmail.org/mailman/listinfo/notmuch
