[PATCH] test-lib: avoid full path to store test results

No reason to use the full path in case this is used externally.

Signed-off-by: Felipe Contreras <redacted>
---
 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

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/t0000-basic.sh

I guess there is nothing wrong with that, though there is no reason not
to use "./" instead of $PWD.

-Peff

On Tue, Oct 30, 2012 at 5:28 AM, Jeff King [off-list ref] wrote:

On Tue, Oct 30, 2012 at 05:12:57AM +0100, Felipe Contreras wrote:

quoted

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

Hi,

Felipe Contreras wrote:

No reason to use the full path in case this is used externally.

Signed-off-by: Felipe Contreras <redacted>

"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

The shell word splitting done in base is a bashism, iow not portable.

Best

2012/10/30, Felipe Contreras [off-list ref]:

-- 
Inviato dal mio dispositivo mobile

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

On Tue, Oct 30, 2012 at 5:46 AM, Jonathan Nieder [off-list ref] wrote:

Felipe Contreras wrote:

quoted

No reason to use the full path in case this is used externally.

Signed-off-by: Felipe Contreras <redacted>

"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

-- 
Felipe Contreras

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 [off-list ref]:

Elia Pinto wrote:

quoted

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

Felipe Contreras wrote:

On Tue, Oct 30, 2012 at 5:46 AM, Jonathan Nieder [off-list ref] wrote:

quoted

Felipe Contreras wrote:

quoted

quoted

No reason to use the full path in case this is used externally.

Signed-off-by: Felipe Contreras <redacted>

"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

On Wed, Oct 31, 2012 at 2:27 AM, Jonathan Nieder [off-list ref] wrote:

Felipe Contreras wrote:

quoted

On Tue, Oct 30, 2012 at 5:46 AM, Jonathan Nieder [off-list ref] wrote:

quoted

Felipe Contreras wrote:

quoted

quoted

quoted

No reason to use the full path in case this is used externally.

Signed-off-by: Felipe Contreras <redacted>

"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

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.

On Wed, Oct 31, 2012 at 3:13 AM, Jonathan Nieder [off-list ref] wrote:

Felipe Contreras wrote:

quoted

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.

[...]

quoted

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

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/msg00009.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

Am 31.10.2012 03:28, schrieb Felipe Contreras:

On Wed, Oct 31, 2012 at 3:13 AM, Jonathan Nieder [off-list ref] wrote:

quoted

Felipe Contreras wrote:

quoted

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

On Wed, Oct 31, 2012 at 7:02 PM, Johannes Sixt [off-list ref] wrote:

Am 31.10.2012 03:28, schrieb Felipe Contreras:

quoted

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

On Wed, Oct 31, 2012 at 07:02:48PM +0100, Johannes Sixt wrote:

Am 31.10.2012 03:28, schrieb Felipe Contreras:

quoted

On Wed, Oct 31, 2012 at 3:13 AM, Jonathan Nieder [off-list ref] wrote:

quoted

Felipe Contreras wrote:

quoted

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

On Fri, Nov 2, 2012 at 2:17 PM, Jeff King [off-list ref] wrote:

On Wed, Oct 31, 2012 at 07:02:48PM +0100, Johannes Sixt wrote:

quoted

Am 31.10.2012 03:28, schrieb Felipe Contreras:

quoted

On Wed, Oct 31, 2012 at 3:13 AM, Jonathan Nieder [off-list ref] wrote:

quoted

Felipe Contreras wrote:

quoted

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

On Fri, Nov 02, 2012 at 04:17:27PM +0100, Felipe Contreras wrote:

quoted

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