Re: t7610-mergetool.sh test failure
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 -- 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: t7610-mergetool.sh test failure
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 -- 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: t7610-mergetool.sh test failure
On Fri, May 27, 2016 at 12:58:15PM -0700, Junio C Hamano wrote: > On Fri, May 27, 2016 at 11:24 AM, Jeff Kingwrote: > > 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 -- 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: t7610-mergetool.sh test failure
On Fri, May 27, 2016 at 11:24 AM, Jeff Kingwrote: > 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. -- 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: t7610-mergetool.sh test failure
On Thu, May 26, 2016 at 11:33:27PM -0700, Junio C Hamano wrote: > Jeff Kingwrites: > > > 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 -- 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: t7610-mergetool.sh test failure
Jeff Kingwrites: > 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. -- 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: t7610-mergetool.sh test failure
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 -- 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: t7610-mergetool.sh test failure
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 > > >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 -- 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: t7610-mergetool.sh test failure
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 > >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 -- 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: t7610-mergetool.sh test failure
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 >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 -- 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: t7610-mergetool.sh test failure
On Tue, May 24, 2016 at 6:45 PM, Junio C Hamanowrote: > 3. find and install mktemp? Sure, but which one? :-) mktemp is not mentioned in POSIX. http://stackoverflow.com/questions/2792675/how-portable-is-mktemp1 -- 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: t7610-mergetool.sh test failure
On Tue, May 24, 2016 at 9:44 AM, Armin Kunaschikwrote: > 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? -- 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