On Sun, Sep 6, 2015 at 7:46 AM, John Keeping <john@xxxxxxxxxxxxx> wrote: > On Sun, Sep 06, 2015 at 05:51:43AM -0400, Eric Sunshine wrote: >> I'm not necessarily advocating this, but think it's worth mentioning >> that an alternate solution would be to fix test_when_finished() to work >> correctly in subshells rather than disallowing its use. This can be done >> by having test_when_finished() collect the cleanup actions in a file >> rather than in a shell variable. >> >> Pros: >> * works in subshells >> * portable across all shells (no Bash special-case) >> * one less rule (restriction) for test writers to remember >> >> Cons: >> * slower >> * could interfere with tests expecting very specific 'trash' directory >> contents (but locating this file under .git might make it safe) > > Another con is that we have to worry about the working directory, and > since we can't reliably detect if we're in a subshell every cleanup > action probably has to be something like: > > ( cd '$(pwd)' && $* ) That's an argument against allowing test_when_finished() inside subshells, in general, isn't it? Subshell callers of test_when_finished(), if correctly written, would already have had to protect against that anyhow, so it's not a "con" of the idea of collecting cleanup actions in a file. Of course, patches 2/5 and 4/5 demonstrate that a couple of those subshell callers did *not* correctly protect against directory (or other) changes which would invalidate their cleanup actions, and were thus buggy anyhow, even if the cleanup actions had been invoked. That's a good argument in favor of disallowing test_when_finished() in subshells, but not a "con" of the suggestion. I'm probably arguing semantics here, thus being annoying, so I'll stop now... > It's certainly possible but it adds another bit of complexity. > > Since there are only 3 out of over 13,000 tests that use this > functionality (and it's quite easy to change them not to) I'm not sure > it's worth supporting it. No argument there. -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html