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

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.


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

Reply via email to