Re: [PATCHv2] add: ignore only ignored files
Jeff King schrieb am 21.11.2014 um 19:01: On Fri, Nov 21, 2014 at 05:08:19PM +0100, Michael J Gruber wrote: git add foo bar adds neither foo nor bar when bar is ignored, but dies to let the user recheck their command invocation. This becomes less helpful when git add foo.* is subject to shell expansion and some of the expanded files are ignored. git add --ignore-errors is supposed to ignore errors when indexing some files and adds the others. It does ignore errors from actual indexing attempts, but does not ignore the error file is ignored as outlined above. This is unexpected. Change git add foo bar to add foo when bar is ignored, but issue a warning and return a failure code as before the change. That is, in the case of trying to add ignored files we now act the same way (with or without --ignore-errors) in which we act for more severe indexing errors when --ignore-errors is specified. Thanks, this looks pretty good to me. I agree with Junio's sense that we should cook it extra long to give people time to react. My sincere thanks go out to Jeff without whom I could not possibly have come up with a patch like this :) :) Sorry if I was being obnoxious before. Sometimes contributors need a gentle push to keep going, but I should know by now that you are not such a person. We were just having fun with each other ;) diff --git a/t/t3700-add.sh b/t/t3700-add.sh index fe274e2..f7ff1f5 100755 --- a/t/t3700-add.sh +++ b/t/t3700-add.sh @@ -91,6 +91,13 @@ test_expect_success 'error out when attempting to add ignored ones without -f' ' ! (git ls-files | grep \\.ig) ' +test_expect_success 'error out when attempting to add ignored ones but add others' ' +touch a.if +test_must_fail git add a.?? +! (git ls-files | grep \\.ig) +(git ls-files | grep a.if) +' I am somewhat allergic to pipes in our test suite, because they can mask errors (especially with a negated grep, because we do not know if they correctly produced any output at all). But I guess this is matching the surrounding code, and it is quite unlikely for `ls-files` to fail in any meaningful way here. So I think it's fine. -Peff I do prefer test_cmp myself, also because it tells you much more in case of a broken test - a failed boolean chain doesn't even tell you where it broke. In this specific case, many more tests would need to be rewriten, though, so I preferred to keep the style of the surrounding code. Michael -- 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: [PATCHv2] add: ignore only ignored files
Torsten Bögershausen schrieb am 22.11.2014 um 15:59: +test_expect_success 'error out when attempting to add ignored ones but add others' ' + touch a.if + test_must_fail git add a.?? + ! (git ls-files | grep \\.ig) + (git ls-files | grep a.if) +' I am somewhat allergic to pipes in our test suite, because they can mask errors (especially with a negated grep, because we do not know if they correctly produced any output at all). But I guess this is matching the surrounding code, and it is quite unlikely for `ls-files` to fail in any meaningful way here. So I think it's fine. -Peff 2 small comments: Why the escaped \\.ig and the unescaped a.if ? Well, the first one is copied straight from another test and the second one is by me. ;) I want to test that no files ending in .ig are added, but that one file a.if is added. Knowing how the test is structured, it is higly unlikely that other people will add a file where the dot in a.if matches something other than a dot, but in the case of .ig I wouldn't be so sure. We could take the extra safety measure and quote a\\.if also, but to me that seems to make the test less readable. The other question, this is a more general one, strikes me every time I see ! grep Should we avoid it by writing test_must_fail instead of ! ? (The current code base has a mixture of both) The following came into my mind when working on another grepy thing, and it may be unnecessary clumsy: test_expect_success 'error out when attempting to add ignored ones but add others' ' touch a.if test_must_fail git add a.?? git ls-files files.txt test_must_fail grep a.ig files.txt /dev/null grep a.if files.txt /dev/null rm files.txt ' (It feels as if there should be a grepnot ;-) I guess that was clarified in the ongoing discussion. The 3rd comment: Thanks for taking this up! Just scratching my own itches mostly these days :) Michael -- 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: [PATCHv2] add: ignore only ignored files
Jeff King p...@peff.net writes: On Sun, Nov 23, 2014 at 10:10:47AM -0800, Junio C Hamano wrote: Jeff King p...@peff.net writes: ... Possibly because I do not know that those instructions are written down anywhere. We usually catch such things in review these days, but there are many inconsistent spots in the existing suite. t/README has this Don't: - use '! git cmd' when you want to make sure the git command exits with failure in a controlled way by calling die(). Instead, use 'test_must_fail git cmd'. This will signal a failure if git dies in an unexpected way (e.g. segfault). On the other hand, don't use test_must_fail for running regular platform commands; just use '! cmd'. Thanks, I did not actually look and relied on my memory, which was obviously wrong. I agree that the instructions there are sufficient. Do we refer to t/README from CodingGuidelines where we tell the developers to always write tests to prevent other people from breaking tomorrow what you did today? If not, perhaps that is what needs to be added. That might make sense. It might also be that Torsten simply overlooked it when asking his question (i.e., there is nothing to fix, documentation is not always read completely, and we can move on). We actually do not have a reference to it anywhere. For now, this should suffice. Documentation/SubmittingPatches | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches index fa71b5f..a3861a6 100644 --- a/Documentation/SubmittingPatches +++ b/Documentation/SubmittingPatches @@ -57,7 +57,8 @@ change, the approach taken by the change, and if relevant how this differs substantially from the prior version, are all good things to have. -Make sure that you have tests for the bug you are fixing. +Make sure that you have tests for the bug you are fixing. See +t/README for guidance of writing tests. When adding a new feature, make sure that you have new tests to show the feature triggers the new behaviour when it should, and to show the -- 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: [PATCHv2] add: ignore only ignored files
On 2014-11-24 18.41, Junio C Hamano wrote: ... Do we refer to t/README from CodingGuidelines where we tell the developers to always write tests to prevent other people from breaking tomorrow what you did today? If not, perhaps that is what needs to be added. That might make sense. It might also be that Torsten simply overlooked it when asking his question (i.e., there is nothing to fix, documentation is not always read completely, and we can move on). Thanks, until yesterday I was unaware of t/README, but now I am :-) -- 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: [PATCHv2] add: ignore only ignored files
On Mon, Nov 24, 2014 at 09:41:00AM -0800, Junio C Hamano wrote: We actually do not have a reference to it anywhere. For now, this should suffice. Documentation/SubmittingPatches | 3 ++- 1 file changed, 2 insertions(+), 1 deletion(-) diff --git a/Documentation/SubmittingPatches b/Documentation/SubmittingPatches index fa71b5f..a3861a6 100644 --- a/Documentation/SubmittingPatches +++ b/Documentation/SubmittingPatches @@ -57,7 +57,8 @@ change, the approach taken by the change, and if relevant how this differs substantially from the prior version, are all good things to have. -Make sure that you have tests for the bug you are fixing. +Make sure that you have tests for the bug you are fixing. See +t/README for guidance of writing tests. That looks a good improvement to me. -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: [PATCHv2] add: ignore only ignored files
Jeff King p...@peff.net writes: ... Possibly because I do not know that those instructions are written down anywhere. We usually catch such things in review these days, but there are many inconsistent spots in the existing suite. t/README has this Don't: - use '! git cmd' when you want to make sure the git command exits with failure in a controlled way by calling die(). Instead, use 'test_must_fail git cmd'. This will signal a failure if git dies in an unexpected way (e.g. segfault). On the other hand, don't use test_must_fail for running regular platform commands; just use '! cmd'. Though it can be improved by justifying just use '! cmd' further with a bit of rationale (e.g. We are not in the business of verifying that world given to us sanely works.), I think the current text is sufficient. Do we refer to t/README from CodingGuidelines where we tell the developers to always write tests to prevent other people from breaking tomorrow what you did today? If not, perhaps that is what needs to be added. -- 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: [PATCHv2] add: ignore only ignored files
On Sun, Nov 23, 2014 at 10:10:47AM -0800, Junio C Hamano wrote: Jeff King p...@peff.net writes: ... Possibly because I do not know that those instructions are written down anywhere. We usually catch such things in review these days, but there are many inconsistent spots in the existing suite. t/README has this Don't: - use '! git cmd' when you want to make sure the git command exits with failure in a controlled way by calling die(). Instead, use 'test_must_fail git cmd'. This will signal a failure if git dies in an unexpected way (e.g. segfault). On the other hand, don't use test_must_fail for running regular platform commands; just use '! cmd'. Thanks, I did not actually look and relied on my memory, which was obviously wrong. I agree that the instructions there are sufficient. Do we refer to t/README from CodingGuidelines where we tell the developers to always write tests to prevent other people from breaking tomorrow what you did today? If not, perhaps that is what needs to be added. That might make sense. It might also be that Torsten simply overlooked it when asking his question (i.e., there is nothing to fix, documentation is not always read completely, and we can move on). -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: [PATCHv2] add: ignore only ignored files
On Sat, Nov 22, 2014 at 10:20:10PM +0100, Torsten Bögershausen wrote: I do not think there is a real _downside_ to using test_must_fail for grep, except that it is a bit more verbose. We may burn CPU cycles It adds a single if/else chain. If your shell does not implement that as a fast builtin, you have bigger performance problems. :) I counted 19 test_must_fail grep under t/*sh, and 201 ! grep. I do not mind a patch to fix them, but with the usual caveat of avoiding stepping on the toes of any topics in flight. It is also fine to leave them until the area is touched. As a general rule for further review of shell scripts can we say ? ! git# incorrect, we don't capture e.g. segfaults of signal test_must_fail grep # correct, but not preferred for new code ! grep # preferred for new code test_must_fail git # correct I think that's true. The snippet from t/README Junio quoted lays it out pretty clearly, I think. If you didn't know there was documentation in t/README that was worth reading before writing tests, then that is the thing I think should go in CodingGuidelines. -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: [PATCHv2] add: ignore only ignored files
+test_expect_success 'error out when attempting to add ignored ones but add others' ' +touch a.if +test_must_fail git add a.?? +! (git ls-files | grep \\.ig) +(git ls-files | grep a.if) +' I am somewhat allergic to pipes in our test suite, because they can mask errors (especially with a negated grep, because we do not know if they correctly produced any output at all). But I guess this is matching the surrounding code, and it is quite unlikely for `ls-files` to fail in any meaningful way here. So I think it's fine. -Peff 2 small comments: Why the escaped \\.ig and the unescaped a.if ? The other question, this is a more general one, strikes me every time I see ! grep Should we avoid it by writing test_must_fail instead of ! ? (The current code base has a mixture of both) The following came into my mind when working on another grepy thing, and it may be unnecessary clumsy: test_expect_success 'error out when attempting to add ignored ones but add others' ' touch a.if test_must_fail git add a.?? git ls-files files.txt test_must_fail grep a.ig files.txt /dev/null grep a.if files.txt /dev/null rm files.txt ' (It feels as if there should be a grepnot ;-) The 3rd comment: Thanks for taking this up! -- 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: [PATCHv2] add: ignore only ignored files
On Sat, Nov 22, 2014 at 03:59:12PM +0100, Torsten Bögershausen wrote: +test_expect_success 'error out when attempting to add ignored ones but add others' ' + touch a.if + test_must_fail git add a.?? + ! (git ls-files | grep \\.ig) + (git ls-files | grep a.if) +' [...] 2 small comments: Why the escaped \\.ig and the unescaped a.if ? I agree that is inconsistent, and I don't see any reason for it. The other question, this is a more general one, strikes me every time I see ! grep Should we avoid it by writing test_must_fail instead of ! ? No. The point of test_must_fail over ! is to check that not only does the command fail, but it fails with a clean exit rather than a signal death. The general philosophy is that this is useful for git (which we are testing), and not for third-party tools that we are using to check our outputs. In other words, we do not expect grep to segfault, and do not need to bother checking it. I do not think there is a real _downside_ to using test_must_fail for grep, except that it is a bit more verbose. And that describes the goal, of course; actual implementation has been less consistent. Possibly because I do not know that those instructions are written down anywhere. We usually catch such things in review these days, but there are many inconsistent spots in the existing suite. The following came into my mind when working on another grepy thing, and it may be unnecessary clumsy: test_expect_success 'error out when attempting to add ignored ones but add others' ' touch a.if test_must_fail git add a.?? git ls-files files.txt test_must_fail grep a.ig files.txt /dev/null grep a.if files.txt /dev/null rm files.txt Right, my allergic to pipes was basically advocating using a tempfile. But as noted above, it should remain ! grep here. And there is no need to redirect the output of grep, as the test suite does it already (in fact, it is preferable not to, because somebody debugging the test with -v will get more output). -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: [PATCHv2] add: ignore only ignored files
On 2014-11-22 20.19, Jeff King wrote: On Sat, Nov 22, 2014 at 03:59:12PM +0100, Torsten Bögershausen wrote: +test_expect_success 'error out when attempting to add ignored ones but add others' ' + touch a.if + test_must_fail git add a.?? + ! (git ls-files | grep \\.ig) + (git ls-files | grep a.if) +' [...] 2 small comments: Why the escaped \\.ig and the unescaped a.if ? I agree that is inconsistent, and I don't see any reason for it. The other question, this is a more general one, strikes me every time I see ! grep Should we avoid it by writing test_must_fail instead of ! ? No. The point of test_must_fail over ! is to check that not only does the command fail, but it fails with a clean exit rather than a signal death. The general philosophy is that this is useful for git (which we are testing), and not for third-party tools that we are using to check our outputs. In other words, we do not expect grep to segfault, and do not need to bother checking it. I do not think there is a real _downside_ to using test_must_fail for grep, except that it is a bit more verbose. We may burn CPU cycles And that describes the goal, of course; actual implementation has been less consistent. Possibly because I do not know that those instructions are written down anywhere. There is a hint in test-lib-functions.sh, but a short notice in CodingGuidelines could be useful, once there is an agreement about grep, which I think we have. We usually catch such things in review these days, but there are many inconsistent spots in the existing suite. The following came into my mind when working on another grepy thing, and it may be unnecessary clumsy: test_expect_success 'error out when attempting to add ignored ones but add others' ' touch a.if test_must_fail git add a.?? git ls-files files.txt test_must_fail grep a.ig files.txt /dev/null grep a.if files.txt /dev/null rm files.txt Right, my allergic to pipes was basically advocating using a tempfile. But as noted above, it should remain ! grep here. And there is no need to redirect the output of grep, as the test suite does it already (in fact, it is preferable not to, because somebody debugging the test with -v will get more output). -Peff I counted 19 test_must_fail grep under t/*sh, and 201 ! grep. As a general rule for further review of shell scripts can we say ? ! git# incorrect, we don't capture e.g. segfaults of signal test_must_fail grep # correct, but not preferred for new code ! grep # preferred for new code test_must_fail git # correct -- 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
[PATCHv2] add: ignore only ignored files
git add foo bar adds neither foo nor bar when bar is ignored, but dies to let the user recheck their command invocation. This becomes less helpful when git add foo.* is subject to shell expansion and some of the expanded files are ignored. git add --ignore-errors is supposed to ignore errors when indexing some files and adds the others. It does ignore errors from actual indexing attempts, but does not ignore the error file is ignored as outlined above. This is unexpected. Change git add foo bar to add foo when bar is ignored, but issue a warning and return a failure code as before the change. That is, in the case of trying to add ignored files we now act the same way (with or without --ignore-errors) in which we act for more severe indexing errors when --ignore-errors is specified. Signed-off-by: Michael J Gruber g...@drmicha.warpmail.net --- As discussed, we change behavior even without the option. I.a.w.: We declare add ignored file as a minor error compared to an actual indexing error. My sincere thanks go out to Jeff without whom I could not possibly have come up with a patch like this :) builtin/add.c | 2 +- t/t3700-add.sh | 8 +++- 2 files changed, 8 insertions(+), 2 deletions(-) diff --git a/builtin/add.c b/builtin/add.c index ae6d3e2..1074e32 100644 --- a/builtin/add.c +++ b/builtin/add.c @@ -284,7 +284,7 @@ static int add_files(struct dir_struct *dir, int flags) for (i = 0; i dir-ignored_nr; i++) fprintf(stderr, %s\n, dir-ignored[i]-name); fprintf(stderr, _(Use -f if you really want to add them.\n)); - die(_(no files added)); + exit_status = 1; } for (i = 0; i dir-nr; i++) diff --git a/t/t3700-add.sh b/t/t3700-add.sh index fe274e2..f7ff1f5 100755 --- a/t/t3700-add.sh +++ b/t/t3700-add.sh @@ -91,6 +91,13 @@ test_expect_success 'error out when attempting to add ignored ones without -f' ' ! (git ls-files | grep \\.ig) ' +test_expect_success 'error out when attempting to add ignored ones but add others' ' + touch a.if + test_must_fail git add a.?? + ! (git ls-files | grep \\.ig) + (git ls-files | grep a.if) +' + test_expect_success 'add ignored ones with -f' ' git add -f a.?? git ls-files --error-unmatch a.ig @@ -311,7 +318,6 @@ cat expect.err \EOF The following paths are ignored by one of your .gitignore files: ignored-file Use -f if you really want to add them. -fatal: no files added EOF cat expect.out \EOF add 'track-this' -- 2.2.0.rc2.293.gc05a35d -- 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: [PATCHv2] add: ignore only ignored files
On Fri, Nov 21, 2014 at 05:08:19PM +0100, Michael J Gruber wrote: git add foo bar adds neither foo nor bar when bar is ignored, but dies to let the user recheck their command invocation. This becomes less helpful when git add foo.* is subject to shell expansion and some of the expanded files are ignored. git add --ignore-errors is supposed to ignore errors when indexing some files and adds the others. It does ignore errors from actual indexing attempts, but does not ignore the error file is ignored as outlined above. This is unexpected. Change git add foo bar to add foo when bar is ignored, but issue a warning and return a failure code as before the change. That is, in the case of trying to add ignored files we now act the same way (with or without --ignore-errors) in which we act for more severe indexing errors when --ignore-errors is specified. Thanks, this looks pretty good to me. I agree with Junio's sense that we should cook it extra long to give people time to react. My sincere thanks go out to Jeff without whom I could not possibly have come up with a patch like this :) :) Sorry if I was being obnoxious before. Sometimes contributors need a gentle push to keep going, but I should know by now that you are not such a person. diff --git a/t/t3700-add.sh b/t/t3700-add.sh index fe274e2..f7ff1f5 100755 --- a/t/t3700-add.sh +++ b/t/t3700-add.sh @@ -91,6 +91,13 @@ test_expect_success 'error out when attempting to add ignored ones without -f' ' ! (git ls-files | grep \\.ig) ' +test_expect_success 'error out when attempting to add ignored ones but add others' ' + touch a.if + test_must_fail git add a.?? + ! (git ls-files | grep \\.ig) + (git ls-files | grep a.if) +' I am somewhat allergic to pipes in our test suite, because they can mask errors (especially with a negated grep, because we do not know if they correctly produced any output at all). But I guess this is matching the surrounding code, and it is quite unlikely for `ls-files` to fail in any meaningful way here. So I think it's fine. -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
