t7610-mergetool.sh test failure

t7610-mergetool.sh test failure

[Armin Kunaschik]

t7610-mergetool.sh fails on systems without mktemp.

mktemp is used in git-mergetool.sh and throws an error when it's not available.
error: mktemp is needed when 'mergetool.writeToTemp' is true

I see 2 options:
1. code around it, write an own mktemp, maybe use the test-mktemp as a basis.
2. disable the test when mktemp is not available

>From my point of view option 2 would be enough.
Any opinions about that?

Peff suggested something like this... works for me.
This patch is probably whitespace damaged... I could not yet figure out a way
to use and preserve tabs with Google mail.

---
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 76306cf..9279bf5 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -589,7 +589,12 @@ test_expect_success 'filenames seen by tools
start with ./' '
        git reset --hard master >/dev/null 2>&1
 '

-test_expect_success 'temporary filenames are used with mergetool.writeToTemp' '
+test_lazy_prereq MKTEMP '
+       tempdir=$(mktemp -d -t foo.XXX) &&
+       test -d "$tempdir"
+'
+
+test_expect_success MKTEMP 'temporary filenames are used with
mergetool.writeToTemp' '
        git checkout -b test16 branch1 &&
        test_config mergetool.writeToTemp true &&
        test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&

Re: t7610-mergetool.sh test failure

[Junio C Hamano]

On Tue, May 24, 2016 at 9:44 AM, Armin Kunaschik
<megabreit@googlemail.com> wrote:
> t7610-mergetool.sh fails on systems without mktemp.
>
> mktemp is used in git-mergetool.sh and throws an error when it's not available.
> error: mktemp is needed when 'mergetool.writeToTemp' is true
>
> I see 2 options:
> 1. code around it, write an own mktemp, maybe use the test-mktemp as a basis.
> 2. disable the test when mktemp is not available

3. find and install mktemp?

Re: t7610-mergetool.sh test failure

[Armin Kunaschik]

On Tue, May 24, 2016 at 6:45 PM, Junio C Hamano <gitster@pobox.com> wrote:

> 3. find and install mktemp?
Sure, but which one? :-) mktemp is not mentioned in POSIX.
http://stackoverflow.com/questions/2792675/how-portable-is-mktemp1

Re: t7610-mergetool.sh test failure

[Jeff King]

On Tue, May 24, 2016 at 09:45:25AM -0700, Junio C Hamano wrote:

> On Tue, May 24, 2016 at 9:44 AM, Armin Kunaschik
> <megabreit@googlemail.com> wrote:
> > t7610-mergetool.sh fails on systems without mktemp.
> >
> > mktemp is used in git-mergetool.sh and throws an error when it's not available.
> > error: mktemp is needed when 'mergetool.writeToTemp' is true
> >
> > I see 2 options:
> > 1. code around it, write an own mktemp, maybe use the test-mktemp as a basis.
> > 2. disable the test when mktemp is not available
> 
> 3. find and install mktemp?

I'd agree more with (3) if it was some critical part of git-mergetool.
But it seems to be a completely optional feature that we use in only one
place, and git-mergetool even detects this case and complains.

So another alternative would be for the test to detect either that
mergetool worked, or that we got the "mktemp is needed" error.

-Peff

Re: t7610-mergetool.sh test failure

[Jeff King]

On Wed, May 25, 2016 at 06:16:15PM -0500, Jeff King wrote:

> On Tue, May 24, 2016 at 09:45:25AM -0700, Junio C Hamano wrote:
> 
> > On Tue, May 24, 2016 at 9:44 AM, Armin Kunaschik
> > <megabreit@googlemail.com> wrote:
> > > t7610-mergetool.sh fails on systems without mktemp.
> > >
> > > mktemp is used in git-mergetool.sh and throws an error when it's not available.
> > > error: mktemp is needed when 'mergetool.writeToTemp' is true
> > >
> > > I see 2 options:
> > > 1. code around it, write an own mktemp, maybe use the test-mktemp as a basis.
> > > 2. disable the test when mktemp is not available
> > 
> > 3. find and install mktemp?
> 
> I'd agree more with (3) if it was some critical part of git-mergetool.
> But it seems to be a completely optional feature that we use in only one
> place, and git-mergetool even detects this case and complains.
> 
> So another alternative would be for the test to detect either that
> mergetool worked, or that we got the "mktemp is needed" error.

BTW, one thing I happened to note while looking at this: running the
test script will write into /tmp (or wherever your $TMPDIR points).
Probably not a big deal, but I wonder if we should be setting $TMPDIR in
our test harness.

-Peff

Re: t7610-mergetool.sh test failure

[David Aguilar]

On Wed, May 25, 2016 at 08:51:14PM -0500, Jeff King wrote:
> On Wed, May 25, 2016 at 06:16:15PM -0500, Jeff King wrote:
> 
> > On Tue, May 24, 2016 at 09:45:25AM -0700, Junio C Hamano wrote:
> > 
> > > On Tue, May 24, 2016 at 9:44 AM, Armin Kunaschik
> > > <megabreit@googlemail.com> wrote:
> > > > t7610-mergetool.sh fails on systems without mktemp.
> > > >
> > > > mktemp is used in git-mergetool.sh and throws an error when it's not available.
> > > > error: mktemp is needed when 'mergetool.writeToTemp' is true
> > > >
> > > > I see 2 options:
> > > > 1. code around it, write an own mktemp, maybe use the test-mktemp as a basis.
> > > > 2. disable the test when mktemp is not available
> > > 
> > > 3. find and install mktemp?
> > 
> > I'd agree more with (3) if it was some critical part of git-mergetool.
> > But it seems to be a completely optional feature that we use in only one
> > place, and git-mergetool even detects this case and complains.
> > 
> > So another alternative would be for the test to detect either that
> > mergetool worked, or that we got the "mktemp is needed" error.
> 
> BTW, one thing I happened to note while looking at this: running the
> test script will write into /tmp (or wherever your $TMPDIR points).
> Probably not a big deal, but I wonder if we should be setting $TMPDIR in
> our test harness.

We already set $HOME and various other variables to carefully
tune the testsuite's behavior, so this sounds like a good idea
IMO.

Would there be any downsides to setting $TMPDIR equal to the
trash directory?

FWIW I set $TMPDIR to the $TEST_OUTPUT_DIRECTORY in test-lib.sh
and was able to run the test suite without any errors.

Is it worth patching?
-- 
David

Re: t7610-mergetool.sh test failure

[Jeff King]

On Thu, May 26, 2016 at 09:40:27PM -0700, David Aguilar wrote:

> > BTW, one thing I happened to note while looking at this: running the
> > test script will write into /tmp (or wherever your $TMPDIR points).
> > Probably not a big deal, but I wonder if we should be setting $TMPDIR in
> > our test harness.
> 
> We already set $HOME and various other variables to carefully
> tune the testsuite's behavior, so this sounds like a good idea
> IMO.
> 
> Would there be any downsides to setting $TMPDIR equal to the
> trash directory?

The only one I can think of is that if something leaves cruft in
$TMPDIR, it could affect later tests that want to `git add`
indiscriminately. It seems unlikely.

OTOH, I do not think putting things in /tmp is hurting anything. I was
mostly just surprised by it.

-Peff

Re: t7610-mergetool.sh test failure

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> The only one I can think of is that if something leaves cruft in
> $TMPDIR, it could affect later tests that want to `git add`
> indiscriminately.

Or "git ls-files -u", "git clean", etc.  I'd mostly worry about a
failed test in which a program dies without a chance to clean up
after itself, and letting the cruft affecting the next test.

> OTOH, I do not think putting things in /tmp is hurting anything. I was
> mostly just surprised by it.

Moving TMPDIR into somewhere under t/ would force us to do more work
to clean things up, and it would probably be a good thing in the
longer term.

I just checked my /tmp, and I see a lot of directories whose name
look like mktemp generated one, with a single socket 's' in them.  I
wouldn't be surprised if they turn out to be from our tests that
expect failure, killing a daemon that does not properly clean after
itself.  People probably would not notice if they are in /tmp, and
if we moved TMPDIR to the trash, we still wouldn't (because running
tests successfully without "-d" option will remove the trash
directory at the end), but if it were dropped somewhere in the
source tree, we have a better chance of noticing it.

Re: t7610-mergetool.sh test failure

[Jeff King]

On Thu, May 26, 2016 at 11:33:27PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > The only one I can think of is that if something leaves cruft in
> > $TMPDIR, it could affect later tests that want to `git add`
> > indiscriminately.
> 
> Or "git ls-files -u", "git clean", etc.  I'd mostly worry about a
> failed test in which a program dies without a chance to clean up
> after itself, and letting the cruft affecting the next test.

Yeah, exactly. OTOH, that could be considered a feature. If one of our
programs is leaving cruft in $TMPDIR, that is probably a bug that should
be fixed.

We wouldn't notice in most cases though (it would depend on some later
test actually caring about the cruft). So your "leave cruft in the
source directory but outside the trash directory" would be better there.
I'd worry slightly that it would cause problems when running tests in
parallel, though. Normal /tmp usage is OK with a global namespace (they
choose unique names), but sometimes things might use /tmp with a
well-known name as a rendezvous point (e.g, for sockets). But I guess
such cases are already broken for running in parallel, since /tmp is a
global namespace.

> I just checked my /tmp, and I see a lot of directories whose name
> look like mktemp generated one, with a single socket 's' in them.  I
> wouldn't be surprised if they turn out to be from our tests that
> expect failure, killing a daemon that does not properly clean after
> itself.  People probably would not notice if they are in /tmp, and
> if we moved TMPDIR to the trash, we still wouldn't (because running
> tests successfully without "-d" option will remove the trash
> directory at the end), but if it were dropped somewhere in the
> source tree, we have a better chance of noticing it.

Hmm. I'm not sure what would create a socket in git except the
credential-cache stuff, and that does not write into /tmp (there was a
GSoC-related series in March to move this, but I don't think it ever
even made it to pu). Maybe the new watchman stuff?

If you have inotify-tools, you can "inotifywait -m /tmp" and run "make
test", but it's quite noisy, as apparently a lot of tested commands use
tempfiles internally. Which perhaps shows that maybe some people would
see a performance regression if we moved from /tmp to a slower
filesystem (e.g., especially people whose git clone is on NFS or
something).

-Peff

Re: t7610-mergetool.sh test failure

[Junio C Hamano]

On Fri, May 27, 2016 at 11:24 AM, Jeff King <peff@peff.net> wrote:
> Which perhaps shows that maybe some people would
> see a performance regression if we moved from /tmp to a slower
> filesystem (e.g., especially people whose git clone is on NFS or
> something).

Yup, but they would be using "--root" already if NFS bothers them;
having TMPDIR pointing somewhere in it would not hurt them, I
would think.

Re: t7610-mergetool.sh test failure

[Jeff King]

On Fri, May 27, 2016 at 12:58:15PM -0700, Junio C Hamano wrote:

> On Fri, May 27, 2016 at 11:24 AM, Jeff King <peff@peff.net> wrote:
> > Which perhaps shows that maybe some people would
> > see a performance regression if we moved from /tmp to a slower
> > filesystem (e.g., especially people whose git clone is on NFS or
> > something).
> 
> Yup, but they would be using "--root" already if NFS bothers them;
> having TMPDIR pointing somewhere in it would not hurt them, I
> would think.

Yeah, but that is not quite the same as "in the source directory" (i.e.,
they would not notice via "git status" later if cruft was left in their
--root path). But I guess people not using "--root" would, and that may
be good enough.

-Peff

Re: t7610-mergetool.sh test failure

[Armin Kunaschik]

Another thread I'm trying to revive... the discussion went away quite a bit
from the suggested patch to conditionally run this one test only when mktemp
is available.

I'll create a patch when there are chances it is accepted.

I could think of a way to replace mktemp with a perl one-liner (or
small script)...
conditionally, when mktemp is not available... maybe in the build process?
As far as I can see, perl is absolutely necessary and can therefore be used to
"solve" the mktemp problem...

...or maybe I should stop bringing this up again :-)

Armin

Re: t7610-mergetool.sh test failure

[Jeff King]

On Wed, Jun 22, 2016 at 06:53:40PM +0200, Armin Kunaschik wrote:

> Another thread I'm trying to revive... the discussion went away quite a bit
> from the suggested patch to conditionally run this one test only when mktemp
> is available.
> 
> I'll create a patch when there are chances it is accepted.
> 
> I could think of a way to replace mktemp with a perl one-liner (or
> small script)...
> conditionally, when mktemp is not available... maybe in the build process?
> As far as I can see, perl is absolutely necessary and can therefore be used to
> "solve" the mktemp problem...
> 
> ...or maybe I should stop bringing this up again :-)

I think perl is necessary for the test suite, but not for git-mergetool
itself. And this is a problem in the script itself, IIRC; so it really
is broken on your system (albeit in a really tiny way), and not just a
test portability thing.

So the viable solutions to me are one of:

  1. Accept that this little part of mergetool doesn't work on systems
     without "mktemp", make sure we fail gracefully (we seem to), and make
     sure the test suite can handle this case (which was the earlier
     patch, I think).

  2. Implement our own git-mktemp (or similar abstraction) in C. We
     already have all the code, so it really is just a thin wrapper.

I don't mind (2), but given the lack of people clamoring for a fix to
mergetool itself, I'm perfectly happy with (1).

-Peff