git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH 5/5] test-lib-functions: detect test_when_finished in subshell

From
Eric Sunshine <sunshine@sunshineco.com>
Date
Sep 6, 2015, 16:22 UTC
Message-ID
<CAPig+cQixsVT-5Ke2Ccedijc1HeBZ6=mbKeeN95V3zExBxSz2A@mail.gmail.com>
In-Reply-To
<20150906114635.GF27660@serenity.lan>
On Sun, Sep 6, 2015 at 7:46 AM, John Keeping <john@keeping.me.uk> wrote:
Show 22 quoted lines
> 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...
Show 5 quoted lines
> 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.
Previous: John KeepingNext: Junio C Hamano
Message 12 of 15 in “[RFC] test_when_finished in subshells”
  1. John KeepingSep 4, 2015
  2. Junio C HamanoSep 4, 2015
  3. Jeff KingSep 5, 2015
  4. 0/5 disallow test_when_finished in subshellsJohn Keeping, Sep 5, 2015
  5. 1/5 t7610: don't use test_config in a subshellJohn Keeping, Sep 5, 2015
  6. 2/5 t5801: don't use test_when_finished in a subshellJohn Keeping, Sep 5, 2015
  7. 3/5 test-lib-functions: support "test_config -C <dir> ..."John Keeping, Sep 5, 2015
  8. 4/5 t7800: don't use test_config in a subshellJohn Keeping, Sep 5, 2015
  9. 5/5 test-lib-functions: detect test_when_finished in subshellJohn Keeping, Sep 5, 2015
  10. Eric SunshineSep 6, 2015
  11. John KeepingSep 6, 2015
  12. Eric SunshineSep 6, 2015
  13. Junio C HamanoSep 5, 2015
  14. John KeepingSep 5, 2015
  15. Junio C HamanoSep 5, 2015

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.