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

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

[Jonathan Nieder]

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.

Add some text to each of git's shell libraries explaining in what
context the fragment is meant to be sourced and remove the shebangs.

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.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Hi,

This patch has only an aesthetic justification but I think it is
basically good.  Last took a brief visit to the list at [1].  More
context at [2].  I'd be happy for any thoughts you have.

Thanks,
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/185079/focus=185106
[2] http://bugs.debian.org/368792
    http://thread.gmane.org/gmane.comp.version-control.git/138253/focus=138560

 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 +++------
 7 files changed, 17 insertions(+), 19 deletions(-)

diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh
index ed630b20..7a0f499b 100644
--- a/git-mergetool--lib.sh
+++ b/git-mergetool--lib.sh
@@ -1,5 +1,4 @@
-#!/bin/sh
-# git-mergetool--lib is a library for common merge tool functions
+# git-mergetool--lib is a shell library for common merge tool functions
 diff_mode() {
 	test "$TOOL_MODE" = diff
 }
diff --git a/git-parse-remote.sh b/git-parse-remote.sh
index b24119d6..739ff772 100644
--- a/git-parse-remote.sh
+++ b/git-parse-remote.sh
@@ -1,4 +1,6 @@
-#!/bin/sh
+# This is a shell library to calculate the remote repository and
+# upstream branch that should be pulled by "git pull" from the current
+# branch.
 
 # git-ls-remote could be called from outside a git managed repository;
 # this would fail in that case and would issue an error message.
diff --git a/git-rebase--am.sh b/git-rebase--am.sh
index c815a241..5d8d451b 100644
--- a/git-rebase--am.sh
+++ b/git-rebase--am.sh
@@ -1,4 +1,5 @@
-#!/bin/sh
+# This shell script fragment is sourced by git-rebase to implement
+# its default, fast, patch-based, non-interactive mode.
 #
 # Copyright (c) 2010 Junio C Hamano.
 #
diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 5812222e..ff00b094 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -1,11 +1,9 @@
-#!/bin/sh
 #
 # Copyright (c) 2006 Johannes E. Schindelin
 
-# SHORT DESCRIPTION
-#
-# This script makes it easy to fix up commits in the middle of a series,
-# and rearrange commits.
+# This shell script fragment is sourced by git-rebase to implement
+# its interactive mode.  "git rebase --interactive" makes it easy
+# to fix up commits in the middle of a series and rearrange commits.
 #
 # The original idea comes from Eric W. Biederman, in
 # http://article.gmane.org/gmane.comp.version-control.git/22407
diff --git a/git-rebase--merge.sh b/git-rebase--merge.sh
index dc599077..0fa0e491 100644
--- a/git-rebase--merge.sh
+++ b/git-rebase--merge.sh
@@ -1,4 +1,6 @@
-#!/bin/sh
+# This shell script fragment is sourced by git-rebase to implement
+# its merge-based non-interactive mode that copes well with renamed
+# files.
 #
 # Copyright (c) 2010 Junio C Hamano.
 #
diff --git a/git-sh-i18n.sh b/git-sh-i18n.sh
index b4575fb3..eff20a08 100644
--- a/git-sh-i18n.sh
+++ b/git-sh-i18n.sh
@@ -1,9 +1,8 @@
-#!/bin/sh
+# This shell library is Git's interface to gettext.sh. See po/README
+# for usage instructions.
 #
 # Copyright (c) 2010 Ævar Arnfjörð Bjarmason
 #
-# This is Git's interface to gettext.sh. See po/README for usage
-# instructions.
 
 # Export the TEXTDOMAIN* data that we need for Git
 TEXTDOMAIN=git
diff --git a/git-sh-setup.sh b/git-sh-setup.sh
index 5d8e4e6c..51362d60 100644
--- a/git-sh-setup.sh
+++ b/git-sh-setup.sh
@@ -1,9 +1,6 @@
-#!/bin/sh
-#
-# This is included in commands that either have to be run from the toplevel
-# of the repository, or with GIT_DIR environment variable properly.
-# If the GIT_DIR does not look like the right correct git-repository,
-# it dies.
+# This shell scriplet is meant to be included by other shell scripts
+# to set up some variables pointing at the normal git directories and
+# a few helper shell functions.
 
 # Having this variable in your environment would break scripts because
 # you would cause "cd" to be taken to unexpected places.  If you
-- 
1.7.9.2.792.g4ea66.dirty

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

[Jeff King]

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

> 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.

>  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

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

[Clemens Buchacher]

Hi Jonathan,

On Thu, Mar 08, 2012 at 06:14:04AM -0600, Jonathan Nieder wrote:
>
> --- a/git-mergetool--lib.sh
> +++ b/git-mergetool--lib.sh
> @@ -1,5 +1,4 @@
> -#!/bin/sh
> -# git-mergetool--lib is a library for common merge tool functions
> +# git-mergetool--lib is a shell library for common merge tool functions

This breaks vim's filetype detection. It can still guess the file type
from the .sh extension, but we strip the extension during the build.
Although one should typically work with the source files, in the past I
did have a look at the installed files on a few occasions. Maybe because
I did not know better, or because the source code was not available to
me. So for me, this outweighs the aesthetic advantages, if any.

Clemens

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

[Marc Branchaud]

On 12-03-09 02:58 AM, Clemens Buchacher wrote:
> Hi Jonathan,
> 
> On Thu, Mar 08, 2012 at 06:14:04AM -0600, Jonathan Nieder wrote:
>>
>> --- a/git-mergetool--lib.sh
>> +++ b/git-mergetool--lib.sh
>> @@ -1,5 +1,4 @@
>> -#!/bin/sh
>> -# git-mergetool--lib is a library for common merge tool functions
>> +# git-mergetool--lib is a shell library for common merge tool functions
> 
> This breaks vim's filetype detection. It can still guess the file type
> from the .sh extension, but we strip the extension during the build.
> Although one should typically work with the source files, in the past I
> did have a look at the installed files on a few occasions. Maybe because
> I did not know better, or because the source code was not available to
> me. So for me, this outweighs the aesthetic advantages, if any.

(I hesitate to suggest the following, but since nobody else is replying to
this thread...)

How about a modeline?  I think the following would work for both emacs and
vi(m?) (I'm not a vi user, so I might have it wrong):

	# -*-mode:shell-script-*-   vi: filetype=sh

(Now I'll wince and duck as people "calmly" discuss the merits of putting
editor-specific modelines in the source code, and especially which editors
deserve such treatment...)

		M.

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

[Jonathan Nieder]

Hi,

Marc Branchaud wrote:
> On 12-03-09 02:58 AM, Clemens Buchacher wrote:
>> On Thu, Mar 08, 2012 at 06:14:04AM -0600, Jonathan Nieder wrote:

>>> +++ b/git-mergetool--lib.sh
>>> @@ -1,5 +1,4 @@
>>> -#!/bin/sh
[...]
>> This breaks vim's filetype detection. It can still guess the file type
>> from the .sh extension, but we strip the extension during the build.
>> Although one should typically work with the source files, in the past I
>> did have a look at the installed files on a few occasions.

It seems moot until someone has had time to deal with the --valgrind
fallout Jeff mentioned.  Here are my thoughts anyway.

vim's filetype detection has been broken for me in other ways, too:
when it sees "#!/bin/sh", by default it assumes that this is a script
intended for Solaris's lowest-common-denominator Bourne shell and
decides to annoyingly flag POSIXisms in red.  So I end up needing to
override vim's default heuristics already.

By the way, my mild dislike for the #! line in shell libraries is
actually rooted in functionality, in a way.  Though it would never
happen in git, I have had too many unhappy experiences of shell
libraries with #!/bin/bash at the top that were used in #!/bin/sh
scripts and broke completely.  So that is where the bad association
comes from.

Steering people towards git-sh-setup.sh when they try to edit
git-sh-setup is just a happy side-effect.

[...]
> How about a modeline?  I think the following would work for both emacs and
> vi(m?) (I'm not a vi user, so I might have it wrong):
>
> 	# -*-mode:shell-script-*-   vi: filetype=sh

A modeline becomes a distraction as people work to get the settings
perfect with respect to line length, indentation style, and changes in
supported editors, and to get them applied consistently as new files
are added.

So from a maintainability point of view, it seems wasteful --- better
to ask people to configure their editor to recognize .sh files,
#!/bin/sh, and files starting with "# " or containing "is a shell
library" on the first line once and be done with it.

Of course I can easily be convinced otherwise.

Thanks for some useful observations and for catching the huge holes
in the change description.

Sincerely,
Jonathan

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

[Clemens Buchacher]

On Mon, Mar 12, 2012 at 02:17:03PM -0500, Jonathan Nieder wrote:
> 
> By the way, my mild dislike for the #! line in shell libraries is
> actually rooted in functionality, in a way.  Though it would never
> happen in git, I have had too many unhappy experiences of shell
> libraries with #!/bin/bash at the top that were used in #!/bin/sh
> scripts and broke completely.  So that is where the bad association
> comes from.

Maybe I am missing something here, but as far as I can see this is an
argument for _keeping_ the #! line, because at least it will give a hint
that a /bin/bash library will not work for a /bin/sh script, whereas
"this is a shell library" can mean almost anything.

Anyways, I can certainly survive without the #! line. So please feel
free to ignore my comments if you feel strongly about this.

Clemens

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

[Junio C Hamano]

Marc Branchaud <marcnarc@xiplink.com> writes:

> On 12-03-09 02:58 AM, Clemens Buchacher wrote:
>> Hi Jonathan,
>> 
>> On Thu, Mar 08, 2012 at 06:14:04AM -0600, Jonathan Nieder wrote:
>>>
>>> --- a/git-mergetool--lib.sh
>>> +++ b/git-mergetool--lib.sh
>>> @@ -1,5 +1,4 @@
>>> -#!/bin/sh
>>> -# git-mergetool--lib is a library for common merge tool functions
>>> +# git-mergetool--lib is a shell library for common merge tool functions
>> 
>> This breaks vim's filetype detection. It can still guess the file type
>> from the .sh extension, but we strip the extension during the build.

Then that _is_ a feature to make it more obvious that the file is
not something you should be editing, no?

> How about a modeline ...
> (Now I'll wince and duck as people "calmly" discuss the merits of putting
> editor-specific modelines in the source code, and especially which editors
> deserve such treatment...)

Please, no "Local Variables:" or "# vim:".  Just don't impose
personal taste in editor settings to others.

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

[Clemens Buchacher]

On Mon, Mar 12, 2012 at 12:50:15PM -0700, Junio C Hamano wrote:
> 
> > On 12-03-09 02:58 AM, Clemens Buchacher wrote:
> >> 
> >> On Thu, Mar 08, 2012 at 06:14:04AM -0600, Jonathan Nieder wrote:
> >>>
> >>> -#!/bin/sh
> >>> -# git-mergetool--lib is a library for common merge tool functions
> >>> +# git-mergetool--lib is a shell library for common merge tool functions
> >> 
> >> This breaks vim's filetype detection. It can still guess the file type
> >> from the .sh extension, but we strip the extension during the build.
> 
> Then that _is_ a feature to make it more obvious that the file is
> not something you should be editing, no?

If that really is an issue, then we should make it write-protected.
Syntax highlighting is primarily useful for reading.