* t7610-mergetool.sh test failure @ 2016-05-24 16:44 Armin Kunaschik 2016-05-24 16:45 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Armin Kunaschik @ 2016-05-24 16:44 UTC (permalink / raw) To: Git List 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\"" && ^ permalink raw reply related [flat|nested] 13+ messages in thread
* Re: t7610-mergetool.sh test failure 2016-05-24 16:44 t7610-mergetool.sh test failure Armin Kunaschik @ 2016-05-24 16:45 ` Junio C Hamano 2016-05-24 16:53 ` Armin Kunaschik 2016-05-25 23:16 ` Jeff King 0 siblings, 2 replies; 13+ messages in thread From: Junio C Hamano @ 2016-05-24 16:45 UTC (permalink / raw) To: Armin Kunaschik; +Cc: Git List 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? ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: t7610-mergetool.sh test failure 2016-05-24 16:45 ` Junio C Hamano @ 2016-05-24 16:53 ` Armin Kunaschik 2016-05-25 23:16 ` Jeff King 1 sibling, 0 replies; 13+ messages in thread From: Armin Kunaschik @ 2016-05-24 16:53 UTC (permalink / raw) To: Junio C Hamano; +Cc: Git List 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: t7610-mergetool.sh test failure 2016-05-24 16:45 ` Junio C Hamano 2016-05-24 16:53 ` Armin Kunaschik @ 2016-05-25 23:16 ` Jeff King 2016-05-26 1:51 ` Jeff King 1 sibling, 1 reply; 13+ messages in thread From: Jeff King @ 2016-05-25 23:16 UTC (permalink / raw) To: Junio C Hamano; +Cc: Armin Kunaschik, Git List 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: t7610-mergetool.sh test failure 2016-05-25 23:16 ` Jeff King @ 2016-05-26 1:51 ` Jeff King 2016-05-27 4:40 ` David Aguilar 0 siblings, 1 reply; 13+ messages in thread From: Jeff King @ 2016-05-26 1:51 UTC (permalink / raw) To: Junio C Hamano; +Cc: Armin Kunaschik, Git List 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: t7610-mergetool.sh test failure 2016-05-26 1:51 ` Jeff King @ 2016-05-27 4:40 ` David Aguilar 2016-05-27 5:00 ` Jeff King 0 siblings, 1 reply; 13+ messages in thread From: David Aguilar @ 2016-05-27 4:40 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, Armin Kunaschik, Git List 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: t7610-mergetool.sh test failure 2016-05-27 4:40 ` David Aguilar @ 2016-05-27 5:00 ` Jeff King 2016-05-27 6:33 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Jeff King @ 2016-05-27 5:00 UTC (permalink / raw) To: David Aguilar; +Cc: Junio C Hamano, Armin Kunaschik, Git List 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: t7610-mergetool.sh test failure 2016-05-27 5:00 ` Jeff King @ 2016-05-27 6:33 ` Junio C Hamano 2016-05-27 18:24 ` Jeff King 0 siblings, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2016-05-27 6:33 UTC (permalink / raw) To: Jeff King; +Cc: David Aguilar, Armin Kunaschik, Git List 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: t7610-mergetool.sh test failure 2016-05-27 6:33 ` Junio C Hamano @ 2016-05-27 18:24 ` Jeff King 2016-05-27 19:58 ` Junio C Hamano 0 siblings, 1 reply; 13+ messages in thread From: Jeff King @ 2016-05-27 18:24 UTC (permalink / raw) To: Junio C Hamano; +Cc: David Aguilar, Armin Kunaschik, Git List 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: t7610-mergetool.sh test failure 2016-05-27 18:24 ` Jeff King @ 2016-05-27 19:58 ` Junio C Hamano 2016-05-27 20:01 ` Jeff King 0 siblings, 1 reply; 13+ messages in thread From: Junio C Hamano @ 2016-05-27 19:58 UTC (permalink / raw) To: Jeff King; +Cc: David Aguilar, Armin Kunaschik, Git List 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. ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: t7610-mergetool.sh test failure 2016-05-27 19:58 ` Junio C Hamano @ 2016-05-27 20:01 ` Jeff King 2016-06-22 16:53 ` Armin Kunaschik 0 siblings, 1 reply; 13+ messages in thread From: Jeff King @ 2016-05-27 20:01 UTC (permalink / raw) To: Junio C Hamano; +Cc: David Aguilar, Armin Kunaschik, Git List 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: t7610-mergetool.sh test failure 2016-05-27 20:01 ` Jeff King @ 2016-06-22 16:53 ` Armin Kunaschik 2016-06-22 17:12 ` Jeff King 0 siblings, 1 reply; 13+ messages in thread From: Armin Kunaschik @ 2016-06-22 16:53 UTC (permalink / raw) To: Jeff King; +Cc: Junio C Hamano, David Aguilar, Git List 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
* Re: t7610-mergetool.sh test failure 2016-06-22 16:53 ` Armin Kunaschik @ 2016-06-22 17:12 ` Jeff King 0 siblings, 0 replies; 13+ messages in thread From: Jeff King @ 2016-06-22 17:12 UTC (permalink / raw) To: Armin Kunaschik; +Cc: Junio C Hamano, David Aguilar, Git List 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 ^ permalink raw reply [flat|nested] 13+ messages in thread
end of thread, other threads:[~2016-06-22 17:12 UTC | newest] Thread overview: 13+ messages (download: mbox.gz / follow: Atom feed) -- links below jump to the message on this page -- 2016-05-24 16:44 t7610-mergetool.sh test failure Armin Kunaschik 2016-05-24 16:45 ` Junio C Hamano 2016-05-24 16:53 ` Armin Kunaschik 2016-05-25 23:16 ` Jeff King 2016-05-26 1:51 ` Jeff King 2016-05-27 4:40 ` David Aguilar 2016-05-27 5:00 ` Jeff King 2016-05-27 6:33 ` Junio C Hamano 2016-05-27 18:24 ` Jeff King 2016-05-27 19:58 ` Junio C Hamano 2016-05-27 20:01 ` Jeff King 2016-06-22 16:53 ` Armin Kunaschik 2016-06-22 17:12 ` Jeff King
This is an external index of several public inboxes, see mirroring instructions on how to clone and mirror all data and code used by this external index.