Re: [PATCH/RFC] remove #!interpreter line from shell libraries

From: Jeff King <peff@peff.net>
Date: Mar 8, 2012, 12:47 UTC
Message-ID: <20120308124748.GA2002@sigill.intra.peff.net>
In-Reply-To: <20120308121403.GA16493@burratino>

On Thu, Mar 08, 2012 at 06:14:04AM -0600, Jonathan Nieder wrote:

Show 6 quoted lines

> As explained in v1.7.0-rc1~9 (2009-11-25), even when a script is
> expected to be sourced instead of executed on its own, a #!/bin/sh
> shebang line can provide useful documentation about what format the
> file is in.  However, it is even clearer to include a comment and no
> shebang at all, to avoid creating the illusion that the indicated
> choice of shell will have any effect at runtime.

I'm OK with this in principle, but I think there are some valgrind fallouts.

One is that I'm slightly confused about why this works at all. In v1.7.0-rc1~9, we were having a problem because the valgrind code in test-lib.sh was confused by the lack of #!-line. Without it, the valgrind code thought the shell lib was not a script, and therefore needed to be wrapped by valgrind.sh and run via valgrind.

You give the reasoning why this is OK here:

> Because these files are not marked executable, removing the #! lines
> would not confuse the valgrind support of our test scripts, so this
> should be safe.  Noticed by lintian.

and that makes sense looking at the valgrind setup code, which checks the executable bit. But that code has always checked the executable bit, and git-mergetool--lib.sh was never marked executable. Why did it fail before v1.7.0-rc1~9 but not now? Were we simply wrong in diagnosing the bug back then?

The second is that later, we tweaked the valgrind code with 36bfb0e (tests: link shell libraries into valgrind directory, 2011-06-17), which uses the shebang to detect those shell libraries. With your patch, t2300 is broken with valgrind. You might not notice it, though, because a previous valgrind run would leave the proper symlinks in place. But if you "git clean" t/valgrind/bin and re-run the test, it will fail.

Show 7 quoted lines

>  git-mergetool--lib.sh      |    3 +--
>  git-parse-remote.sh        |    4 +++-
>  git-rebase--am.sh          |    3 ++-
>  git-rebase--interactive.sh |    8 +++-----
>  git-rebase--merge.sh       |    4 +++-
>  git-sh-i18n.sh             |    5 ++---
>  git-sh-setup.sh            |    9 +++------

If we are interested in the aesthetic argument, then many shell libraries in t/ could use the same treatment.

-Peff