Re: [PATCH] test-lib: avoid full path to store test results
On Wed, Oct 31, 2012 at 07:02:48PM +0100, Johannes Sixt wrote: Am 31.10.2012 03:28, schrieb Felipe Contreras: On Wed, Oct 31, 2012 at 3:13 AM, Jonathan Nieder jrnie...@gmail.com wrote: Felipe Contreras wrote: It's all fun and games to write explanations for things, but it's not that easy when you want those explanations to be actually true, and corrent--you have to spend time to make sure of that. That's why it's useful for the patch submitter to write them, asking for help when necessary. As a bonus, it helps reviewers understand the effect of the patch. Bugs averted! Yeah, that would be nice. Too bad I don't have that information, and have _zero_ motivation to go and get it for you. Just to clarify: That information is not just for Jonathan, but for everyone on this list and those who dig the history a year down the road. Contributors who have _zero_ motiviation to find out that information are not welcome here because they cause friction and take away time from many others for _zero_ gain. And me, who is trying to figure out what to do with this patch. It is presented on its own, outside of a series, with only the description no reason not to do this. But AFAICT, it is _required_ for the tests in the remote-hg series to work. Isn't that kind of an important motivation? Yet it is not in the commit message, nor does the remote-hg series indicate that it should be built on top. Or am I wrong that the one is dependent on the other? -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] test-lib: avoid full path to store test results
On Fri, Nov 2, 2012 at 2:17 PM, Jeff King p...@peff.net wrote: On Wed, Oct 31, 2012 at 07:02:48PM +0100, Johannes Sixt wrote: Am 31.10.2012 03:28, schrieb Felipe Contreras: On Wed, Oct 31, 2012 at 3:13 AM, Jonathan Nieder jrnie...@gmail.com wrote: Felipe Contreras wrote: It's all fun and games to write explanations for things, but it's not that easy when you want those explanations to be actually true, and corrent--you have to spend time to make sure of that. That's why it's useful for the patch submitter to write them, asking for help when necessary. As a bonus, it helps reviewers understand the effect of the patch. Bugs averted! Yeah, that would be nice. Too bad I don't have that information, and have _zero_ motivation to go and get it for you. Just to clarify: That information is not just for Jonathan, but for everyone on this list and those who dig the history a year down the road. Contributors who have _zero_ motiviation to find out that information are not welcome here because they cause friction and take away time from many others for _zero_ gain. And me, who is trying to figure out what to do with this patch. It is presented on its own, outside of a series, with only the description no reason not to do this. Yeah, because I think it stands on its own. But I'll include it in the remote-hg patch series, I already have it queued up. But AFAICT, it is _required_ for the tests in the remote-hg series to work. Isn't that kind of an important motivation? It's not _requied_, you will see that error at the end, and the aggregate results would all be 0, but the tests would still work. Yet it is not in the commit message, nor does the remote-hg series indicate that it should be built on top. Or am I wrong that the one is dependent on the other? They are not dependent. 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] test-lib: avoid full path to store test results
On Fri, Nov 02, 2012 at 04:17:27PM +0100, Felipe Contreras wrote: And me, who is trying to figure out what to do with this patch. It is presented on its own, outside of a series, with only the description no reason not to do this. Yeah, because I think it stands on its own. But I'll include it in the remote-hg patch series, I already have it queued up. Thanks. -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] test-lib: avoid full path to store test results
On 10/30/2012 11:17 PM, Elia Pinto wrote: Thanks. I know that posix support these usages, but exists some traditional shell that not support it. True, but those shells are not POSIX shells -- the major example that comes to mind is the accursed Solaris /bin/sh. Since Git assumes a POSIX shell in its scripts and testsuite, use of any POSIX feature should be fine -- until someone can show a real-world POSIX shell that (likely due to a bug) fails to grasp such feature, in which case a pragmatic workaround is needed. Oh, and BTW, there are talks (and mostly consensus) among the Autotools developers to start requiring a POSIX shell in the configure scripts and Makefile recipes in the near future: http://lists.gnu.org/archive/html/bug-autoconf/2012-06/msg9.html And also, related: http://lists.gnu.org/archive/html/automake/2012-08/msg00046.html http://lists.gnu.org/archive/html/coreutils/2012-10/msg00127.html These are described in the autoconf manual, last time i have checked. As the construct ; export var = x should be portable, but it is not. I don't think POSIX requires that to be portable. If this is important these days i don't know. I hope the above helps to clarify the matter a little. Regards, Stefano -- 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] test-lib: avoid full path to store test results
Am 31.10.2012 03:28, schrieb Felipe Contreras: On Wed, Oct 31, 2012 at 3:13 AM, Jonathan Nieder jrnie...@gmail.com wrote: Felipe Contreras wrote: It's all fun and games to write explanations for things, but it's not that easy when you want those explanations to be actually true, and corrent--you have to spend time to make sure of that. That's why it's useful for the patch submitter to write them, asking for help when necessary. As a bonus, it helps reviewers understand the effect of the patch. Bugs averted! Yeah, that would be nice. Too bad I don't have that information, and have _zero_ motivation to go and get it for you. Just to clarify: That information is not just for Jonathan, but for everyone on this list and those who dig the history a year down the road. Contributors who have _zero_ motiviation to find out that information are not welcome here because they cause friction and take away time from many others for _zero_ gain. -- 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] test-lib: avoid full path to store test results
On Wed, Oct 31, 2012 at 7:02 PM, Johannes Sixt j...@kdbg.org wrote: Am 31.10.2012 03:28, schrieb Felipe Contreras: Yeah, that would be nice. Too bad I don't have that information, and have _zero_ motivation to go and get it for you. Just to clarify: That information is not just for Jonathan, but for everyone on this list and those who dig the history a year down the road. Information that nobody has requested but Johannes. Contributors who have _zero_ motiviation to find out that information are not welcome here because they cause friction and take away time from many others for _zero_ gain. Fine, stay with the broken code. 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] test-lib: avoid full path to store test results
The shell word splitting done in base is a bashism, iow not portable.
Best
2012/10/30, Felipe Contreras felipe.contre...@gmail.com:
No reason to use the full path in case this is used externally.
Signed-off-by: Felipe Contreras felipe.contre...@gmail.com
---
t/test-lib.sh | 3 ++-
1 file changed, 2 insertions(+), 1 deletion(-)
diff --git a/t/test-lib.sh b/t/test-lib.sh
index 514282c..5a3d665 100644
--- a/t/test-lib.sh
+++ b/t/test-lib.sh
@@ -389,7 +389,8 @@ test_done () {
then
test_results_dir=$TEST_OUTPUT_DIRECTORY/test-results
mkdir -p $test_results_dir
- test_results_path=$test_results_dir/${0%.sh}-$$.counts
+ base=${0##*/}
+ test_results_path=$test_results_dir/${base%.sh}-$$.counts
cat $test_results_path -EOF
total $test_count
--
1.8.0
--
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
--
Inviato dal mio dispositivo mobile
--
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] test-lib: avoid full path to store test results
Elia Pinto wrote:
The shell word splitting done in base is a bashism, iow not portable.
No, ${varname##glob} is in POSIX and we already use it here and there.
See Documentation/CodingGuidelines:
- We use ${parameter#word} and its [#%] siblings, and their
doubled longest matching form.
Thanks for looking the patch over.
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] test-lib: avoid full path to store test results
Thanks. I know that posix support these usages, but exists some
traditional shell that not support it. These are described in the
autoconf manual, last time i have checked. As the construct ; export
var = x should be portable, but it is not. If this is important these
days i don't know.
Best
2012/10/30, Jonathan Nieder jrnie...@gmail.com:
Elia Pinto wrote:
The shell word splitting done in base is a bashism, iow not portable.
No, ${varname##glob} is in POSIX and we already use it here and there.
See Documentation/CodingGuidelines:
- We use ${parameter#word} and its [#%] siblings, and their
doubled longest matching form.
Thanks for looking the patch over.
Jonathan
--
Inviato dal mio dispositivo mobile
--
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] test-lib: avoid full path to store test results
Felipe Contreras wrote: On Tue, Oct 30, 2012 at 5:46 AM, Jonathan Nieder jrnie...@gmail.com wrote: Felipe Contreras wrote: No reason to use the full path in case this is used externally. Signed-off-by: Felipe Contreras felipe.contre...@gmail.com No reason not to is not a reason to do anything. What symptoms does this prevent? Could you describe the benefit of this patch in a paragraph starting Now you can ...? ./test-lib.sh: line 394: /home/felipec/dev/git/t/test-results//home/felipec/dev/git/contrib/remote-hg/test-21865.counts: No such file or directory Ok, so a description for this patch is test: use test's basename to name results file Running a test using its full path produces an error: $ ~/dev/git/contrib/remote-hg/test-21865.sh [...] ./test-lib.sh: line 394: /home/felipec/dev/t/\ test-results//home/felipec/dev/git/contrib/remote-hg/\ test-21865.counts: No such file or directory In --tee and --valgrind modes we already use the basename to name the .out and .exit files; this patch teaches the test-lib to name the .counts file the same way. That is still not enough to tell if it is a good change, though. Should the test results for contrib/remote-hg/test-* be included with the results for t/t*.sh when I run make aggregate-results? Before 60d02ccc, git-svn had its own testsuite under contrib/, with glue in contrib/git-svn/t/lib-git-svn.sh to use test-lib --- maybe that code could provide some inspiration for questions like these. Hope that helps, 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] test-lib: avoid full path to store test results
On Wed, Oct 31, 2012 at 2:27 AM, Jonathan Nieder jrnie...@gmail.com wrote: Felipe Contreras wrote: On Tue, Oct 30, 2012 at 5:46 AM, Jonathan Nieder jrnie...@gmail.com wrote: Felipe Contreras wrote: No reason to use the full path in case this is used externally. Signed-off-by: Felipe Contreras felipe.contre...@gmail.com No reason not to is not a reason to do anything. What symptoms does this prevent? Could you describe the benefit of this patch in a paragraph starting Now you can ...? ./test-lib.sh: line 394: /home/felipec/dev/git/t/test-results//home/felipec/dev/git/contrib/remote-hg/test-21865.counts: No such file or directory Ok, so a description for this patch is test: use test's basename to name results file Is this solving an actual problem or is it just something nice to do? Like in all good novels, one has to read more find out... Running a test using its full path produces an error: I'm not sure what that even means. Do you mean this produces an error? % make -C t $PWD/t9902-completion.sh Well, sure it does, but this patch doesn't fix that. If you want a precise explanation of what kind of usages are enabled by this patch, that would require some work, and no I haven't done it, and no, I'm not sure. $ ~/dev/git/contrib/remote-hg/test-21865.sh [...] ./test-lib.sh: line 394: /home/felipec/dev/t/\ test-results//home/felipec/dev/git/contrib/remote-hg/\ test-21865.counts: No such file or directory Except that I didn't do this. So the fact that this happens is an assumption, and I'm not willing to make that. Most likely if somebody does that they are doing something wrong; they didn't define the TESTDIR variable (or something like that). It's all fun and games to write explanations for things, but it's not that easy when you want those explanations to be actually true, and corrent--you have to spend time to make sure of that. In --tee and --valgrind modes we already use the basename to name the .out and .exit files; this patch teaches the test-lib to name the .counts file the same way. I don't see the point of listing each and every place where this already happens. As a matter of fact, the base-name is used in other places as well, and just saying This is already done in other places, is more than enough. But who says they are not the ones doing it wrong? Maybe this part of the code is right, and it's the others that need fixing. I don't see how saying Others are doing it makes the patch better or worse in any way. There might also be different reasons for why they do it that doesn't apply here. That is still not enough to tell if it is a good change, though. Should the test results for contrib/remote-hg/test-* be included with the results for t/t*.sh when I run make aggregate-results? Before 60d02ccc, git-svn had its own testsuite under contrib/, with glue in contrib/git-svn/t/lib-git-svn.sh to use test-lib --- maybe that code could provide some inspiration for questions like these. Or maybe they are the ones that should look for inspiration in contrib/remote-hg. The patch is obviously correct; it's generally good not to name files with slashes in them, and $0 is not guaranteed not to have slashes. Even if you run all the tests inside the 't' directory, this script is not only used by git, and others might want sub-directories, and not thousands of tests on the same directory like git. Either way, if obvious fixes that are one-liners require an essay for you, I give up. 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] test-lib: avoid full path to store test results
Felipe Contreras wrote: It's all fun and games to write explanations for things, but it's not that easy when you want those explanations to be actually true, and corrent--you have to spend time to make sure of that. That's why it's useful for the patch submitter to write them, asking for help when necessary. As a bonus, it helps reviewers understand the effect of the patch. Bugs averted! [...] Either way, if obvious fixes that are one-liners require an essay for you, I give up. I guess it is fair to call a reasonable subject line plus a couple of sentences an essay. Yes, obvious fixes especially require that, since the bug caused by an obvious fix is one of the worst kinds. -- 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] test-lib: avoid full path to store test results
On Wed, Oct 31, 2012 at 3:13 AM, Jonathan Nieder jrnie...@gmail.com wrote: Felipe Contreras wrote: It's all fun and games to write explanations for things, but it's not that easy when you want those explanations to be actually true, and corrent--you have to spend time to make sure of that. That's why it's useful for the patch submitter to write them, asking for help when necessary. As a bonus, it helps reviewers understand the effect of the patch. Bugs averted! Yeah, that would be nice. Too bad I don't have that information, and have _zero_ motivation to go and get it for you. [...] Either way, if obvious fixes that are one-liners require an essay for you, I give up. I guess it is fair to call a reasonable subject line plus a couple of sentences an essay. Yes, obvious fixes especially require that, since the bug caused by an obvious fix is one of the worst kinds. Yes, I've written essays for one-line fixes, in the Linux kernel, where details matter, and things are not so obvious. This is not the case here. This is as obvious and simple as things get. If there was a problem with it, somebody would have found it by now. Let not the perfect be the enemy of the good. Or do. Up to you. -- 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] test-lib: avoid full path to store test results
On Tue, Oct 30, 2012 at 05:12:57AM +0100, Felipe Contreras wrote: No reason to use the full path in case this is used externally. I think it is not just no reason to, but it is actively wrong to use a full path, as we do not take care to mkdir -p the intervening path components. However, this never comes up in practice, because all of the test scripts assume you are running them from the test directory (i.e., they will fail otherwise because they will not find ./test-lib.sh). Is this in support of putting remote-hg tests in contrib/? I had expected you to just put export TEST_DIRECTORY=$(pwd)/../../../t . $TEST_DIRECTORY/test-lib.sh into the test script in contrib/remote-hg/t. I guess you are doing something like: cd ../../../t ../contrib/remote-hg/t/twhatever... but the former seems much simpler to invoke (and if the goal is to get your test-results in the right place, setting TEST_OUTPUT_DIRECTORY is the best way to do that). If this is part of the remote-hg series, I'd prefer to just see it as part of the re-roll. It's much easier to evaluate it in context. Or are you really just doing: cd git/t $PWD/t-basic.sh I guess there is nothing wrong with that, though there is no reason not to use ./ instead of $PWD. -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] test-lib: avoid full path to store test results
On Tue, Oct 30, 2012 at 5:28 AM, Jeff King p...@peff.net wrote: On Tue, Oct 30, 2012 at 05:12:57AM +0100, Felipe Contreras wrote: No reason to use the full path in case this is used externally. I think it is not just no reason to, but it is actively wrong to use a full path, as we do not take care to mkdir -p the intervening path components. However, this never comes up in practice, because all of the test scripts assume you are running them from the test directory (i.e., they will fail otherwise because they will not find ./test-lib.sh). Is this in support of putting remote-hg tests in contrib/? I had expected you to just put export TEST_DIRECTORY=$(pwd)/../../../t . $TEST_DIRECTORY/test-lib.sh If there was a single script and we didn't want reports, sure, but this is not too bad: TESTS := $(wildcard test*.sh) export T := $(addprefix $(CURDIR)/,$(TESTS)) export MAKE := $(MAKE) -e export PATH := $(CURDIR):$(PATH) test: $(MAKE) -C ../../t $@ $(TESTS): $(MAKE) -C ../../t $(CURDIR)/$@ .PHONY: $(TESTS) I just sent the new remote-hg patch series with that. 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] test-lib: avoid full path to store test results
Hi, Felipe Contreras wrote: No reason to use the full path in case this is used externally. Signed-off-by: Felipe Contreras felipe.contre...@gmail.com No reason not to is not a reason to do anything. What symptoms does this prevent? Could you describe the benefit of this patch in a paragraph starting Now you can ...? Thanks, 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
