{"thread":{"id":"29884","subject":"[PATCH/RFC] remove #!interpreter line from shell libraries","startedAt":"2012-03-08T12:14:04Z","lastAt":"2012-03-13T19:09:49Z","messageCount":8,"participants":["Jonathan Nieder","Jeff King","Clemens Buchacher","Marc Branchaud","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"186430","messageId":"20120308121403.GA16493@burratino","threadId":"29884","inReplyTo":null,"subject":"[PATCH/RFC] remove #!interpreter line from shell libraries","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-08T12:14:04Z","receivedAt":"2012-03-08T12:14:04Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"As explained in v1.7.0-rc1~9 (2009-11-25), even when a script is\nexpected to be sourced instead of executed on its own, a #!/bin/sh\nshebang line can provide useful documentation about what format the\nfile is in.  However, it is even clearer to include a comment and no\nshebang at all, to avoid creating the illusion that the indicated\nchoice of shell will have any effect at runtime.\n\nAdd some text to each of git's shell libraries explaining in what\ncontext the fragment is meant to be sourced and remove the shebangs.\n\nBecause these files are not marked executable, removing the #! lines\nwould not confuse the valgrind support of our test scripts, so this\nshould be safe.  Noticed by lintian.\n\nSigned-off-by: Jonathan Nieder <jrnieder@gmail.com>\n---\nHi,\n\nThis patch has only an aesthetic justification but I think it is\nbasically good.  Last took a brief visit to the list at [1].  More\ncontext at [2].  I'd be happy for any thoughts you have.\n\nThanks,\nJonathan\n\n[1] http://thread.gmane.org/gmane.comp.version-control.git/185079/focus=185106\n[2] http://bugs.debian.org/368792\n    http://thread.gmane.org/gmane.comp.version-control.git/138253/focus=138560\n\n git-mergetool--lib.sh      |    3 +--\n git-parse-remote.sh        |    4 +++-\n git-rebase--am.sh          |    3 ++-\n git-rebase--interactive.sh |    8 +++-----\n git-rebase--merge.sh       |    4 +++-\n git-sh-i18n.sh             |    5 ++---\n git-sh-setup.sh            |    9 +++------\n 7 files changed, 17 insertions(+), 19 deletions(-)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex ed630b20..7a0f499b 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -1,5 +1,4 @@\n-#!/bin/sh\n-# git-mergetool--lib is a library for common merge tool functions\n+# git-mergetool--lib is a shell library for common merge tool functions\n diff_mode() {\n \ttest \"$TOOL_MODE\" = diff\n }\ndiff --git a/git-parse-remote.sh b/git-parse-remote.sh\nindex b24119d6..739ff772 100644\n--- a/git-parse-remote.sh\n+++ b/git-parse-remote.sh\n@@ -1,4 +1,6 @@\n-#!/bin/sh\n+# This is a shell library to calculate the remote repository and\n+# upstream branch that should be pulled by \"git pull\" from the current\n+# branch.\n \n # git-ls-remote could be called from outside a git managed repository;\n # this would fail in that case and would issue an error message.\ndiff --git a/git-rebase--am.sh b/git-rebase--am.sh\nindex c815a241..5d8d451b 100644\n--- a/git-rebase--am.sh\n+++ b/git-rebase--am.sh\n@@ -1,4 +1,5 @@\n-#!/bin/sh\n+# This shell script fragment is sourced by git-rebase to implement\n+# its default, fast, patch-based, non-interactive mode.\n #\n # Copyright (c) 2010 Junio C Hamano.\n #\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 5812222e..ff00b094 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -1,11 +1,9 @@\n-#!/bin/sh\n #\n # Copyright (c) 2006 Johannes E. Schindelin\n \n-# SHORT DESCRIPTION\n-#\n-# This script makes it easy to fix up commits in the middle of a series,\n-# and rearrange commits.\n+# This shell script fragment is sourced by git-rebase to implement\n+# its interactive mode.  \"git rebase --interactive\" makes it easy\n+# to fix up commits in the middle of a series and rearrange commits.\n #\n # The original idea comes from Eric W. Biederman, in\n # http://article.gmane.org/gmane.comp.version-control.git/22407\ndiff --git a/git-rebase--merge.sh b/git-rebase--merge.sh\nindex dc599077..0fa0e491 100644\n--- a/git-rebase--merge.sh\n+++ b/git-rebase--merge.sh\n@@ -1,4 +1,6 @@\n-#!/bin/sh\n+# This shell script fragment is sourced by git-rebase to implement\n+# its merge-based non-interactive mode that copes well with renamed\n+# files.\n #\n # Copyright (c) 2010 Junio C Hamano.\n #\ndiff --git a/git-sh-i18n.sh b/git-sh-i18n.sh\nindex b4575fb3..eff20a08 100644\n--- a/git-sh-i18n.sh\n+++ b/git-sh-i18n.sh\n@@ -1,9 +1,8 @@\n-#!/bin/sh\n+# This shell library is Git's interface to gettext.sh. See po/README\n+# for usage instructions.\n #\n # Copyright (c) 2010 Ævar Arnfjörð Bjarmason\n #\n-# This is Git's interface to gettext.sh. See po/README for usage\n-# instructions.\n \n # Export the TEXTDOMAIN* data that we need for Git\n TEXTDOMAIN=git\ndiff --git a/git-sh-setup.sh b/git-sh-setup.sh\nindex 5d8e4e6c..51362d60 100644\n--- a/git-sh-setup.sh\n+++ b/git-sh-setup.sh\n@@ -1,9 +1,6 @@\n-#!/bin/sh\n-#\n-# This is included in commands that either have to be run from the toplevel\n-# of the repository, or with GIT_DIR environment variable properly.\n-# If the GIT_DIR does not look like the right correct git-repository,\n-# it dies.\n+# This shell scriplet is meant to be included by other shell scripts\n+# to set up some variables pointing at the normal git directories and\n+# a few helper shell functions.\n \n # Having this variable in your environment would break scripts because\n # you would cause \"cd\" to be taken to unexpected places.  If you\n-- \n1.7.9.2.792.g4ea66.dirty\n"},{"id":"186435","messageId":"20120308124748.GA2002@sigill.intra.peff.net","threadId":"29884","inReplyTo":"20120308121403.GA16493@burratino","subject":"Re: [PATCH/RFC] remove #!interpreter line from shell libraries","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2012-03-08T12:47:48Z","receivedAt":"2012-03-08T12:47:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Mar 08, 2012 at 06:14:04AM -0600, Jonathan Nieder wrote:\n\n> As explained in v1.7.0-rc1~9 (2009-11-25), even when a script is\n> expected to be sourced instead of executed on its own, a #!/bin/sh\n> shebang line can provide useful documentation about what format the\n> file is in.  However, it is even clearer to include a comment and no\n> shebang at all, to avoid creating the illusion that the indicated\n> choice of shell will have any effect at runtime.\n\nI'm OK with this in principle, but I think there are some valgrind\nfallouts.\n\nOne is that I'm slightly confused about why this works at all.  In\nv1.7.0-rc1~9, we were having a problem because the valgrind code in\ntest-lib.sh was confused by the lack of #!-line. Without it, the\nvalgrind code thought the shell lib was not a script, and therefore\nneeded to be wrapped by valgrind.sh and run via valgrind.\n\nYou give the reasoning why this is OK here:\n\n> Because these files are not marked executable, removing the #! lines\n> would not confuse the valgrind support of our test scripts, so this\n> should be safe.  Noticed by lintian.\n\nand that makes sense looking at the valgrind setup code, which checks the\nexecutable bit. But that code has always checked the executable bit, and\ngit-mergetool--lib.sh was never marked executable. Why did it fail\nbefore v1.7.0-rc1~9 but not now? Were we simply wrong in diagnosing the\nbug back then?\n\nThe second is that later, we tweaked the valgrind code with 36bfb0e\n(tests: link shell libraries into valgrind directory, 2011-06-17), which\nuses the shebang to detect those shell libraries. With your patch,\nt2300 is broken with valgrind. You might not notice it, though, because\na previous valgrind run would leave the proper symlinks in place. But if\nyou \"git clean\" t/valgrind/bin and re-run the test, it will fail.\n\n>  git-mergetool--lib.sh      |    3 +--\n>  git-parse-remote.sh        |    4 +++-\n>  git-rebase--am.sh          |    3 ++-\n>  git-rebase--interactive.sh |    8 +++-----\n>  git-rebase--merge.sh       |    4 +++-\n>  git-sh-i18n.sh             |    5 ++---\n>  git-sh-setup.sh            |    9 +++------\n\nIf we are interested in the aesthetic argument, then many shell\nlibraries in t/ could use the same treatment.\n\n-Peff\n"},{"id":"186537","messageId":"20120309075820.GA15985@ecki","threadId":"29884","inReplyTo":"20120308121403.GA16493@burratino","subject":"Re: [PATCH/RFC] remove #!interpreter line from shell libraries","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-03-09T07:58:20Z","receivedAt":"2012-03-09T07:58:20Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"Hi Jonathan,\n\nOn Thu, Mar 08, 2012 at 06:14:04AM -0600, Jonathan Nieder wrote:\n>\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -1,5 +1,4 @@\n> -#!/bin/sh\n> -# git-mergetool--lib is a library for common merge tool functions\n> +# git-mergetool--lib is a shell library for common merge tool functions\n\nThis breaks vim's filetype detection. It can still guess the file type\nfrom the .sh extension, but we strip the extension during the build.\nAlthough one should typically work with the source files, in the past I\ndid have a look at the installed files on a few occasions. Maybe because\nI did not know better, or because the source code was not available to\nme. So for me, this outweighs the aesthetic advantages, if any.\n\nClemens\n"},{"id":"186758","messageId":"4F5E4618.10102@xiplink.com","threadId":"29884","inReplyTo":"20120309075820.GA15985@ecki","subject":"Re: [PATCH/RFC] remove #!interpreter line from shell libraries","fromName":"Marc Branchaud","fromEmail":"marcnarc@xiplink.com","sentAt":"2012-03-12T18:53:12Z","receivedAt":"2012-03-12T18:53:12Z","isPatch":true,"sender":{"key":"marcnarc@xiplink.com","avatar":"https://avatars.githubusercontent.com/u/14980203?v=4"},"body":"On 12-03-09 02:58 AM, Clemens Buchacher wrote:\n> Hi Jonathan,\n> \n> On Thu, Mar 08, 2012 at 06:14:04AM -0600, Jonathan Nieder wrote:\n>>\n>> --- a/git-mergetool--lib.sh\n>> +++ b/git-mergetool--lib.sh\n>> @@ -1,5 +1,4 @@\n>> -#!/bin/sh\n>> -# git-mergetool--lib is a library for common merge tool functions\n>> +# git-mergetool--lib is a shell library for common merge tool functions\n> \n> This breaks vim's filetype detection. It can still guess the file type\n> from the .sh extension, but we strip the extension during the build.\n> Although one should typically work with the source files, in the past I\n> did have a look at the installed files on a few occasions. Maybe because\n> I did not know better, or because the source code was not available to\n> me. So for me, this outweighs the aesthetic advantages, if any.\n\n(I hesitate to suggest the following, but since nobody else is replying to\nthis thread...)\n\nHow about a modeline?  I think the following would work for both emacs and\nvi(m?) (I'm not a vi user, so I might have it wrong):\n\n\t# -*-mode:shell-script-*-   vi: filetype=sh\n\n(Now I'll wince and duck as people \"calmly\" discuss the merits of putting\neditor-specific modelines in the source code, and especially which editors\ndeserve such treatment...)\n\n\t\tM.\n"},{"id":"186763","messageId":"20120312191703.GB21817@burratino","threadId":"29884","inReplyTo":"4F5E4618.10102@xiplink.com","subject":"Re: [PATCH/RFC] remove #!interpreter line from shell libraries","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2012-03-12T19:17:03Z","receivedAt":"2012-03-12T19:17:03Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nMarc Branchaud wrote:\n> On 12-03-09 02:58 AM, Clemens Buchacher wrote:\n>> On Thu, Mar 08, 2012 at 06:14:04AM -0600, Jonathan Nieder wrote:\n\n>>> +++ b/git-mergetool--lib.sh\n>>> @@ -1,5 +1,4 @@\n>>> -#!/bin/sh\n[...]\n>> This breaks vim's filetype detection. It can still guess the file type\n>> from the .sh extension, but we strip the extension during the build.\n>> Although one should typically work with the source files, in the past I\n>> did have a look at the installed files on a few occasions.\n\nIt seems moot until someone has had time to deal with the --valgrind\nfallout Jeff mentioned.  Here are my thoughts anyway.\n\nvim's filetype detection has been broken for me in other ways, too:\nwhen it sees \"#!/bin/sh\", by default it assumes that this is a script\nintended for Solaris's lowest-common-denominator Bourne shell and\ndecides to annoyingly flag POSIXisms in red.  So I end up needing to\noverride vim's default heuristics already.\n\nBy the way, my mild dislike for the #! line in shell libraries is\nactually rooted in functionality, in a way.  Though it would never\nhappen in git, I have had too many unhappy experiences of shell\nlibraries with #!/bin/bash at the top that were used in #!/bin/sh\nscripts and broke completely.  So that is where the bad association\ncomes from.\n\nSteering people towards git-sh-setup.sh when they try to edit\ngit-sh-setup is just a happy side-effect.\n\n[...]\n> How about a modeline?  I think the following would work for both emacs and\n> vi(m?) (I'm not a vi user, so I might have it wrong):\n>\n> \t# -*-mode:shell-script-*-   vi: filetype=sh\n\nA modeline becomes a distraction as people work to get the settings\nperfect with respect to line length, indentation style, and changes in\nsupported editors, and to get them applied consistently as new files\nare added.\n\nSo from a maintainability point of view, it seems wasteful --- better\nto ask people to configure their editor to recognize .sh files,\n#!/bin/sh, and files starting with \"# \" or containing \"is a shell\nlibrary\" on the first line once and be done with it.\n\nOf course I can easily be convinced otherwise.\n\nThanks for some useful observations and for catching the huge holes\nin the change description.\n\nSincerely,\nJonathan\n"},{"id":"186769","messageId":"7vy5r5r43s.fsf@alter.siamese.dyndns.org","threadId":"29884","inReplyTo":"4F5E4618.10102@xiplink.com","subject":"Re: [PATCH/RFC] remove #!interpreter line from shell libraries","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-03-12T19:50:15Z","receivedAt":"2012-03-12T19:50:15Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Marc Branchaud <marcnarc@xiplink.com> writes:\n\n> On 12-03-09 02:58 AM, Clemens Buchacher wrote:\n>> Hi Jonathan,\n>> \n>> On Thu, Mar 08, 2012 at 06:14:04AM -0600, Jonathan Nieder wrote:\n>>>\n>>> --- a/git-mergetool--lib.sh\n>>> +++ b/git-mergetool--lib.sh\n>>> @@ -1,5 +1,4 @@\n>>> -#!/bin/sh\n>>> -# git-mergetool--lib is a library for common merge tool functions\n>>> +# git-mergetool--lib is a shell library for common merge tool functions\n>> \n>> This breaks vim's filetype detection. It can still guess the file type\n>> from the .sh extension, but we strip the extension during the build.\n\nThen that _is_ a feature to make it more obvious that the file is\nnot something you should be editing, no?\n\n> How about a modeline ...\n> (Now I'll wince and duck as people \"calmly\" discuss the merits of putting\n> editor-specific modelines in the source code, and especially which editors\n> deserve such treatment...)\n\nPlease, no \"Local Variables:\" or \"# vim:\".  Just don't impose\npersonal taste in editor settings to others.\n"},{"id":"186792","messageId":"20120312213847.GA4958@ecki","threadId":"29884","inReplyTo":"7vy5r5r43s.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH/RFC] remove #!interpreter line from shell libraries","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-03-12T21:38:47Z","receivedAt":"2012-03-12T21:38:47Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Mon, Mar 12, 2012 at 12:50:15PM -0700, Junio C Hamano wrote:\n> \n> > On 12-03-09 02:58 AM, Clemens Buchacher wrote:\n> >> \n> >> On Thu, Mar 08, 2012 at 06:14:04AM -0600, Jonathan Nieder wrote:\n> >>>\n> >>> -#!/bin/sh\n> >>> -# git-mergetool--lib is a library for common merge tool functions\n> >>> +# git-mergetool--lib is a shell library for common merge tool functions\n> >> \n> >> This breaks vim's filetype detection. It can still guess the file type\n> >> from the .sh extension, but we strip the extension during the build.\n> \n> Then that _is_ a feature to make it more obvious that the file is\n> not something you should be editing, no?\n\nIf that really is an issue, then we should make it write-protected.\nSyntax highlighting is primarily useful for reading.\n"},{"id":"186894","messageId":"20120313190949.GB8074@ecki","threadId":"29884","inReplyTo":"20120312191703.GB21817@burratino","subject":"Re: [PATCH/RFC] remove #!interpreter line from shell libraries","fromName":"Clemens Buchacher","fromEmail":"drizzd@aon.at","sentAt":"2012-03-13T19:09:49Z","receivedAt":"2012-03-13T19:09:49Z","isPatch":true,"sender":{"key":"drizzd@gmx.net","avatar":"https://avatars.githubusercontent.com/u/59082?v=4"},"body":"On Mon, Mar 12, 2012 at 02:17:03PM -0500, Jonathan Nieder wrote:\n> \n> By the way, my mild dislike for the #! line in shell libraries is\n> actually rooted in functionality, in a way.  Though it would never\n> happen in git, I have had too many unhappy experiences of shell\n> libraries with #!/bin/bash at the top that were used in #!/bin/sh\n> scripts and broke completely.  So that is where the bad association\n> comes from.\n\nMaybe I am missing something here, but as far as I can see this is an\nargument for _keeping_ the #! line, because at least it will give a hint\nthat a /bin/bash library will not work for a /bin/sh script, whereas\n\"this is a shell library\" can mean almost anything.\n\nAnyways, I can certainly survive without the #! line. So please feel\nfree to ignore my comments if you feel strongly about this.\n\nClemens\n"}]}