{"thread":{"id":"20716","subject":"gitosis-lite","startedAt":"2009-08-24T12:28:57Z","lastAt":"2009-08-26T05:01:21Z","messageCount":10,"participants":["Sitaram Chamarty","Jakub Narebski","Shawn O. Pearce"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"121626","messageId":"2e24e5b90908240528v56fdf30cq4c046fca321a3b17@mail.gmail.com","threadId":"20716","inReplyTo":null,"subject":"gitosis-lite","fromName":"Sitaram Chamarty","fromEmail":"sitaramc@gmail.com","sentAt":"2009-08-24T12:28:57Z","receivedAt":"2009-08-24T12:28:57Z","isPatch":false,"sender":{"key":"sitaramc@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43316?v=4"},"body":"[Tommi: I chose this name to reflect that it really owes its existence to\ngitosis for the basic ideas etc.  However, if you have any objections I\nwould be quite happy to find another name.  Just let me know...]\n\n\n\nHello all,\n\nI created a new project called gitosis-lite, which combines\nthe essential pieces of gitosis with the per-branch\npermissions stuff in the example in the howto directory of\ngit.git.\n\nThe config file is different, (there's an annotated example\nyou can look at).\n\nThe \"why\" and the \"what\" are all at\nhttp://github.com/sitaramc/gitosis-lite\n\n-- \nSitaram\n"},{"id":"121628","messageId":"m3ab1pmk1c.fsf@localhost.localdomain","threadId":"20716","inReplyTo":"2e24e5b90908240528v56fdf30cq4c046fca321a3b17@mail.gmail.com","subject":"Re: gitosis-lite","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-08-24T13:13:53Z","receivedAt":"2009-08-24T13:13:53Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Sitaram Chamarty <sitaramc@gmail.com> writes:\n\n> Hello all,\n> \n> I created a new project called gitosis-lite, which combines\n> the essential pieces of gitosis with the per-branch\n> permissions stuff in the example in the howto directory of\n> git.git.\n> \n> The config file is different, (there's an annotated example\n> you can look at).\n> \n> The \"why\" and the \"what\" are all at\n> http://github.com/sitaramc/gitosis-lite\n\nCould you add information about this tool (perhaps after confirmation\n/ deciding on project name[1]) to Git Wiki page\n  http://git.or.cz/gitwiki/InterfacesFrontendsAndTools\nsomewhere below Gitosis?  Please do not forget to include that it is\nwritten in Perl; see other entries for example.\n\n\nYou wrote in project's README.markdown that you were inspired by\nGitosis (which requires Python and python-setuptools) and\nDocumentation/howto/update-hook-example.txt (which uses bash).\nWhy not contrib/hooks/update-paranoid (which is written in Perl)?\n\nUsing Perl code for configuration is simple and fast, but not very\nsecure.  Why not use git config format (via \"git config -l -z\" like in\ngitweb), or YAML or JSON (or Storable)?  Well, YAML might be overkill.\n\nBTW. if you blog about gitosis-lite (http://sitaramc.blogspot.com/)\nit could be picked up by Perl Iron Man Blogging challenge, and you\ncould get wider review.\n\n[1]: gitosis-lite doesn't look like CPAN-y name.  Git::Admin perlhaps?\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"121638","messageId":"2e24e5b90908240735n5f754c87u960493c92769c0a8@mail.gmail.com","threadId":"20716","inReplyTo":"m3ab1pmk1c.fsf@localhost.localdomain","subject":"Re: gitosis-lite","fromName":"Sitaram Chamarty","fromEmail":"sitaramc@gmail.com","sentAt":"2009-08-24T14:35:19Z","receivedAt":"2009-08-24T14:35:19Z","isPatch":false,"sender":{"key":"sitaramc@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43316?v=4"},"body":"On Mon, Aug 24, 2009 at 6:43 PM, Jakub Narebski<jnareb@gmail.com> wrote:\n\n> Could you add information about this tool (perhaps after confirmation\n> / deciding on project name[1]) to Git Wiki page\n>  http://git.or.cz/gitwiki/InterfacesFrontendsAndTools\n> somewhere below Gitosis?  Please do not forget to include that it is\n> written in Perl; see other entries for example.\n\nWill do; though I'll probably wait until at least one person\nother than me has used it :)\n\n> You wrote in project's README.markdown that you were inspired by\n> Gitosis (which requires Python and python-setuptools) and\n> Documentation/howto/update-hook-example.txt (which uses bash).\n> Why not contrib/hooks/update-paranoid (which is written in Perl)?\n\nHmmm several reasons.  To start with, it appears to me that\nupdate-paranoid assumes each user has his own ACL file, and\nI'm going the other way -- you'll notice I have exactly one\nfile as a single ACL source for many *projects*, leave alone\nusers.\n\nSecondly, I want to stick to the gitosis philosophy -- it\nhas served very well, and I'm not sure how \"in use\"\nupdate-paranoid is.\n\nThirdly, I'm not comfortable having a hook be so complex.\nThe whole thing is 10 lines shorter than all my code put\ntogether :)\n\nAnd finally, it's far too complex for me, leave alone some\nof my users.  I count 6 pipe opens in there, and the ACLs\nare read by git cat-file or something :) I'll be honest: I\ncame away feeling very stupid after trying to read and\nunderstand that program.  It was... humbling :(\n\n> Using Perl code for configuration is simple and fast, but not very\n> secure.  Why not use git config format (via \"git config -l -z\" like in\n\nNot secure in what sense?  Only the \"git admin\" (whoever\nowns the account in which gitosis-lite is running) will be\nable to generate it, and only scripts that run with his\nauthority (by way of hooks in repos he owns) can run it.\n\nIt cannot do what he did not intend to do, and what he wants\nto do he can do anyway, whether it's JSON or perl.\n\nExcept for umasking the file, I don't see anything I missed.\nCould you help me understand?\n\n> gitweb), or YAML or JSON (or Storable)?  Well, YAML might be overkill.\n\nOne of the objectives is to work *as is* on any Unix system\nthat managed to install git (which implies Perl).  So, no\nJSON or YAML.  Storable would be fine (I think it's also\npart of core perl) so if I find a compelling reason I don't\nmind switching to that.\n\nAll I really have is a nested hash to be saved and restored\n-- nothing more complex, no objects, no blessed refs, etc.\n\n> BTW. if you blog about gitosis-lite (http://sitaramc.blogspot.com/)\n> it could be picked up by Perl Iron Man Blogging challenge, and you\n> could get wider review.\n\nWill do; thanks.  I did not know my blog got picked up by\nanything; till date it has never come up on a google search\nor a blog search anywhere, and it's more of a rant-box +\nannotated bookmarking service for myself :)\n\n> [1]: gitosis-lite doesn't look like CPAN-y name.  Git::Admin perlhaps?\n\nToo presumptuous for someone like me -- makes it sound like\nthe \"one true way to Admin git\" :)\n\nThank you very much for your comments!\n\n-- \nSitaram\n"},{"id":"121644","messageId":"20090824151051.GB1033@spearce.org","threadId":"20716","inReplyTo":"2e24e5b90908240735n5f754c87u960493c92769c0a8@mail.gmail.com","subject":"Re: gitosis-lite","fromName":"Shawn O. Pearce","fromEmail":"spearce@spearce.org","sentAt":"2009-08-24T15:10:51Z","receivedAt":"2009-08-24T15:10:51Z","isPatch":false,"sender":{"key":"spearce@spearce.org","avatar":"https://avatars.githubusercontent.com/u/34844?v=4"},"body":"Sitaram Chamarty <sitaramc@gmail.com> wrote:\n> On Mon, Aug 24, 2009 at 6:43 PM, Jakub Narebski<jnareb@gmail.com> wrote:\n> > Why not contrib/hooks/update-paranoid (which is written in Perl)?\n> \n> Hmmm several reasons.  To start with, it appears to me that\n> update-paranoid assumes each user has his own ACL file, and\n> I'm going the other way -- you'll notice I have exactly one\n> file as a single ACL source for many *projects*, leave alone\n> users.\n\nThat is true, update-paranoid uses an ACL file per user, because\nwe approached it from the perspective of \"what can this user do\"\nrather than \"what is allowed in this project\".\n\nWe later ran into needing groups, which meant that in practice the\nper-user ACL files really just enumerated what groups they were\na member of, and the group ACL files enumerated what they were\nallowed to do in projects.\n\nI can certainly see how this might be backwards, and someone would\nprefer the other direction, a per-project listing or something.\n \n> Secondly, I want to stick to the gitosis philosophy -- it\n> has served very well, and I'm not sure how \"in use\"\n> update-paranoid is.\n\nWell, my prior job used it heavily, and I think is still using it,\na year after I left.  My current job would use it, except that we\nwent and wrote Gerrit Code Review instead.  :-)\n \n> Thirdly, I'm not comfortable having a hook be so complex.\n> The whole thing is 10 lines shorter than all my code put\n> together :)\n\nYes, update-paranoid is a big script.  It does a lot.  It not only\nchecks \"can you write in this repository, on this branch\" but it\nalso checks that every commit matches your own email address(es),\nand it can also check file path edits, to see if you can modify\nthe files that were changed.\n \n> And finally, it's far too complex for me, leave alone some\n> of my users.  I count 6 pipe opens in there,\n\nNot surprised, it needs to read a lot of data from git to do its\njob correctly.  It has to enumerate all new commits, any commits\nwhich may be discarded by the update (non-fast-forward), as well\nas scan the diff to see which paths were affected.\n\n> and the ACLs\n> are read by git cat-file or something\n\nBecause I didn't want to check out a copy of the ACLs to the local\ndisk just to read them into the hook.  The hook pulls them directly\nout of a bare \"admin\" repository, making it easier to push changes\ninto the repository and have them take effect immediately.\n\nI know Gitosis handles this by having a magic hook in the admin\nrepository that copies the file out.  update-paranoid just reads\nit from git.\n\n> I'll be honest: I\n> came away feeling very stupid after trying to read and\n> understand that program.  It was... humbling :(\n\n*sigh* That's not good, the hook is meant as a practical example,\nif it was too complex to understand, I did a poor job of writing it.\n \n-- \nShawn.\n"},{"id":"121646","messageId":"200908241743.15841.jnareb@gmail.com","threadId":"20716","inReplyTo":"2e24e5b90908240735n5f754c87u960493c92769c0a8@mail.gmail.com","subject":"Re: gitosis-lite","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-08-24T15:43:12Z","receivedAt":"2009-08-24T15:43:12Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, 24 August 2009, Sitaram Chamarty wrote:\n> On Mon, Aug 24, 2009 at 6:43 PM, Jakub Narebski<jnareb@gmail.com> wrote:\n> \n> > Could you add information about this tool (perhaps after confirmation\n> > / deciding on project name[1]) to Git Wiki page\n> >  http://git.or.cz/gitwiki/InterfacesFrontendsAndTools\n> > somewhere below Gitosis?  Please do not forget to include that it is\n> > written in Perl; see other entries for example.\n> \n> Will do; though I'll probably wait until at least one person\n> other than me has used it :)\n\nOn the other hand having the link in IFAT wiki page could help you\ngain more users of gitosis-lite (name pending ;-)\n\n> > You wrote in project's README.markdown that you were inspired by\n> > Gitosis (which requires Python and python-setuptools) and\n> > Documentation/howto/update-hook-example.txt (which uses bash).\n> > Why not contrib/hooks/update-paranoid (which is written in Perl)?\n> \n> Hmmm several reasons.  To start with, it appears to me that\n> update-paranoid assumes each user has his own ACL file, and\n> I'm going the other way -- you'll notice I have exactly one\n> file as a single ACL source for many *projects*, leave alone\n> users.\n\nWell, gitosis uses single SSH user: 'git' (by default, at least).\n\nAlso from what I understand update-paranoid uses the same set of ACLs\nfor all projects (well, depending how $acl_git and $acl_branch are\ndefined).\n \n> Secondly, I want to stick to the gitosis philosophy -- it\n> has served very well, and I'm not sure how \"in use\"\n> update-paranoid is.\n\nI wonder about this too.\n \n> Thirdly, I'm not comfortable having a hook be so complex.\n> The whole thing is 10 lines shorter than all my code put\n> together :)\n\nThe update-paranoid hook has inline POD documentation, which adds to\nits size (I think that your gitosis-lite (name pending) should also\nuse this Perl literate programming convention).  On the other hand\nthis documentation is not, unfortunately, up to date; for example\nit allows to control which directories you can change, which is not\ndocumented in POD.\n\n> And finally, it's far too complex for me, leave alone some\n> of my users.  I count 6 pipe opens in there, and the ACLs\n> are read by git cat-file or something :)\n\nThat's because update-paranoid, like gitosis, uses configuration in\n$scl_repo repository.  git-cat-file is low level (plumbing) equivalent\nof git-show (porcelain).\n\nThose pipelines are required for strict (paranoid) access control that\nupdate-paranoid uses: checking that committers are on allowed list,\nthat taggers are on allowed list, that branches points to commits, that\ntags points to tag objects, etc.\n\n> I'll be honest: I \n> came away feeling very stupid after trying to read and\n> understand that program.  It was... humbling :(\n\nOn the other hand it uses subroutine prototypes (unnecessary, and not\nrecommended), old-style bareword filehandles instead of lexically scoped\nfilehandles (\"open I, ...\" vs \"open my $fh, ...\")\n \n> > Using Perl code for configuration is simple and fast, but not very\n> > secure.  Why not use git config format (via \"git config -l -z\" like in\n> \n> Not secure in what sense?  Only the \"git admin\" (whoever\n> owns the account in which gitosis-lite is running) will be\n> able to generate it, and only scripts that run with his\n> authority (by way of hooks in repos he owns) can run it.\n> \n> It cannot do what he did not intend to do, and what he wants\n> to do he can do anyway, whether it's JSON or perl.\n> \n> Except for umasking the file, I don't see anything I missed.\n> Could you help me understand?\n\nAh, thats O.K. then.\n\n> \n> > gitweb), or YAML or JSON (or Storable)?  Well, YAML might be overkill.\n> \n> One of the objectives is to work *as is* on any Unix system\n> that managed to install git (which implies Perl).  So, no\n> JSON or YAML.  Storable would be fine (I think it's also\n> part of core perl) so if I find a compelling reason I don't\n> mind switching to that.\n\nOne reason to not use Storable is that it is binary.  Another that format\nis not exactly stable (but it contains version info, so it is backwards\ncompatibile I think).\n\nAs for dependencies: some people on #perl told me to not worry about\ndependencies because there is PAR :-)\n\n> \n> All I really have is a nested hash to be saved and restored\n> -- nothing more complex, no objects, no blessed refs, etc.\n\nJSON would be good then ;-)\n\n> \n> > BTW. if you blog about gitosis-lite (http://sitaramc.blogspot.com/)\n> > it could be picked up by Perl Iron Man Blogging challenge, and you\n> > could get wider review.\n> \n> Will do; thanks.  I did not know my blog got picked up by\n> anything; till date it has never come up on a google search\n> or a blog search anywhere, and it's more of a rant-box +\n> annotated bookmarking service for myself :)\n\nI don't know if it would be picked; still, you can try to submit it to\n(join) http://ironman.enlightenedperl.org; note that only posts which\ncontain one of magic words ('perl', 'ironman', ...) would get picked by\nthis planet.\n\n> \n> > [1]: gitosis-lite doesn't look like CPAN-y name.  Git::Admin perlhaps?\n> \n> Too presumptuous for someone like me -- makes it sound like\n> the \"one true way to Admin git\" :)\n\nGit::Auth perhaps (authentication and authorization)?  \nGit::AccessControl?\n\nYou can always ask on #perl (also on freenode).\n-- \nJakub Narebski\nPoland\n"},{"id":"121660","messageId":"m363cdm4pm.fsf@localhost.localdomain","threadId":"20716","inReplyTo":"2e24e5b90908240528v56fdf30cq4c046fca321a3b17@mail.gmail.com","subject":"Re: gitosis-lite","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-08-24T18:44:55Z","receivedAt":"2009-08-24T18:44:55Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Sitaram Chamarty <sitaramc@gmail.com> writes:\n\n> I created a new project called gitosis-lite, which combines\n> the essential pieces of gitosis with the per-branch\n> permissions stuff in the example in the howto directory of\n> git.git.\n\nAs for the name: gitness, gitamine, gitrify,... ;-)\n \n> The config file is different, (there's an annotated example\n> you can look at).\n> \n> The \"why\" and the \"what\" are all at\n> http://github.com/sitaramc/gitosis-lite\n\nA few comments about the code, taking gl-auth-command as example.\n\n> #!/usr/bin/perl -w\n>\n> use strict;\n\nWouldn't it be better to use \"use warnings\" instead of 'perl -w'?\n\n> # === auth-command ===\n> # the command that GL users actually run\n> \n> # part of the gitosis-lite (GL) suite\n> \n> # how run:      via sshd, being listed in \"command=\" in ssh authkeys\n> # when:         every login by a GL user\n> # input:        $1 is GL username, plus $SSH_ORIGINAL_COMMAND\n> # output:\n> # security:\n> #     - currently, we just make some basic checks, copied from gitosis\n>\n> # robustness:\n>\n> # other notes:\n\nIt would be, I think, better if you have used POD for such\ndocumentation.  One would be able to generate manpage using pod2man,\nand it is no less readable in source code.  See e.g. perl/Git.pm or\ncontrib/hooks/update-paranoid.\n\n> our $GL_ADMINDIR;\n> our $GL_CONF;\n> our $GL_KEYDIR;\n> our $GL_CONF_COMPILED;\n> our $REPO_BASE;\n> our %repos;\n\nWhy is the reason behind using 'our' instead of 'my' here?\n\n> # first, fix the biggest gripe I have with gitosis, a 1-line change\n> my $user=$ENV{GL_USER}=shift;       # there; now that's available everywhere!\n\nEh?  This is standalone script, isn't it?  Shouldn't it be\n\n  my $user = $ENV{GL_USER} = $ARGV[0];       # there; now that's available everywhere!\n\n> my $perm = 'W'; $perm = 'R' if $verb =~ $R_COMMANDS;\n\nEither split it into two lines, or use ?: confitional operator:\n\n  my $perm = ($verb =~ $R_COMMANDS ? 'R' : 'W');\n\n> open(LOG, \">>\", \"$GL_ADMINDIR/log\");\n> print LOG \"\\n\", scalar(localtime), \" $ENV{SSH_ORIGINAL_COMMAND} $user\\n\";\n> close(LOG);\n\nIt is better practice to use lexical variables instead of barewords\nfor filehandles:\n\n  if (open my $logfh, \">>\", \"$GL_ADMINDIR/log\") {\n  \tprint $logfh \"\\n\", scalar(localtime), \" $ENV{SSH_ORIGINAL_COMMAND} $user\\n\";\n  \tclose $logfh;\n  }\n\nDon't forget to check for error.\n\n> $repo = \"'$REPO_BASE/$repo.git'\";\n> exec(\"git\", \"shell\", \"-c\", \"$verb $repo\");\n\nThat's not enough.  You have to shell-quote $repo, like in gitweb or\nusing String::ShellQuote module, or somehow use list form to pass\narguments to git-shell.  You protect here againts spaces in filename,\nbut not againts \"'\" (single quote) and for show shells \"!\"\n(exclamation mark).\n\n-- \nJakub Narebski\nPoland\nShadeHawk on #git\n"},{"id":"121697","messageId":"2e24e5b90908242000t212347fbkec6a01396d784736@mail.gmail.com","threadId":"20716","inReplyTo":"20090824151051.GB1033@spearce.org","subject":"Re: gitosis-lite","fromName":"Sitaram Chamarty","fromEmail":"sitaramc@gmail.com","sentAt":"2009-08-25T03:00:26Z","receivedAt":"2009-08-25T03:00:26Z","isPatch":false,"sender":{"key":"sitaramc@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43316?v=4"},"body":"On Mon, Aug 24, 2009 at 8:40 PM, Shawn O. Pearce<spearce@spearce.org> wrote:\n> Sitaram Chamarty <sitaramc@gmail.com> wrote:\n\n>> I'll be honest: I\n>> came away feeling very stupid after trying to read and\n>> understand that program.  It was... humbling :(\n\n> *sigh* That's not good, the hook is meant as a practical example,\n> if it was too complex to understand, I did a poor job of writing it.\n\nHmmm no.  It was just doing too much to grasp in one\nreading, especially by someone whose perl seems to have\nrusted a wee bit, if you've seen Jakub's reply :)\n\nI took a slightly longer look after your mail and I grokked\nit a lot better.  And wow... I guess you called it\n\"paranoid\" for a reason :-)\n\nIn the environments I'm catering to, every commit will be\nreviewed by someone else (usually someone more experienced,\nand having a bigger picture), so \"you're not allowed to\ntouch this file\" type of thing will all come out.\n\nAll I need is to make sure the important branches can only\nbe pushed to by certain people; the \"process\" will take care\nof the rest.\n"},{"id":"121712","messageId":"2e24e5b90908242253v411ad5f3t8a2802079914d0bf@mail.gmail.com","threadId":"20716","inReplyTo":"m363cdm4pm.fsf@localhost.localdomain","subject":"Re: gitosis-lite","fromName":"Sitaram Chamarty","fromEmail":"sitaramc@gmail.com","sentAt":"2009-08-25T05:53:30Z","receivedAt":"2009-08-25T05:53:30Z","isPatch":false,"sender":{"key":"sitaramc@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43316?v=4"},"body":"[took Tommi out of cc; he must be getting enough mail as is...]\n\nOn Tue, Aug 25, 2009 at 12:14 AM, Jakub Narebski<jnareb@gmail.com> wrote:\n\n> A few comments about the code, taking gl-auth-command as example.\n\n> Wouldn't it be better to use \"use warnings\" instead of 'perl -w'?\n\nI'm not sure what is the minimum perl required for git\nitself.  Has it needed perl > 5.6.0 for more than a year at\nleast?  The only real difference between these two is scope,\nwhich is a non-issue here, so I played safe.\n\n> It would be, I think, better if you have used POD for such\n> documentation.  One would be able to generate manpage using pod2man,\n> and it is no less readable in source code.  See e.g. perl/Git.pm or\n> contrib/hooks/update-paranoid.\n\nHmm... I've been spoiled by Markdown's sane bullet list\nhandling.  Visually, POD forces everything other than code\nto be flush left -- any sort of list is definitely less\nreadable in source code as a result.  IMHO of course.\n\n>> our $GL_ADMINDIR;\n>> our $GL_CONF;\n>> our $GL_KEYDIR;\n>> our $GL_CONF_COMPILED;\n>> our $REPO_BASE;\n>> our %repos;\n>\n> Why is the reason behind using 'our' instead of 'my' here?\n\nThey are assigned values in some file being \"do\"-ed, so they\ncan't be lexical scoped.  However, I found a few others that\nwere holdovers from an earlier version.  Fixed; thanks for\ncatching that.\n\n>> # first, fix the biggest gripe I have with gitosis, a 1-line change\n>> my $user=$ENV{GL_USER}=shift;       # there; now that's available everywhere!\n>\n> Eh?  This is standalone script, isn't it?  Shouldn't it be\n>\n>   my $user = $ENV{GL_USER} = $ARGV[0];       # there; now that's available everywhere!\n\nHmm... I didn't know there was a difference, other than\ndepleting @ARGV, if you're outside a subroutine.  I'll take\na relook at it.\n\n>> my $perm = 'W'; $perm = 'R' if $verb =~ $R_COMMANDS;\n>\n> Either split it into two lines, or use ?: confitional operator:\n>\n>   my $perm = ($verb =~ $R_COMMANDS ? 'R' : 'W');\n\nmuch nicer... Fixed, thanks.\n\n>> open(LOG, \">>\", \"$GL_ADMINDIR/log\");\n>> print LOG \"\\n\", scalar(localtime), \" $ENV{SSH_ORIGINAL_COMMAND} $user\\n\";\n>> close(LOG);\n>\n> It is better practice to use lexical variables instead of barewords\n> for filehandles:\n\nGood catch; thanks!  I guess I'm showing my age :)  Fixed\nall of them!\n\n> Don't forget to check for error.\n\nHmm.. well I'm still debating if a log file write error\nshould block git access / push, but there were two more\nimportant closes (again in gl-compile-conf) that were\nunguarded.  Fixed, thanks.\n\n>> $repo = \"'$REPO_BASE/$repo.git'\";\n>> exec(\"git\", \"shell\", \"-c\", \"$verb $repo\");\n>\n> That's not enough.  You have to shell-quote $repo, like in gitweb or\n> using String::ShellQuote module, or somehow use list form to pass\n> arguments to git-shell.  You protect here againts spaces in filename,\n> but not againts \"'\" (single quote) and for show shells \"!\"\n> (exclamation mark).\n\nIt'll never get here.  It'll die much earlier if the\nreponame does not match a much stricter pattern.  Maybe too\nstrict, actually: ^[0-9a-zA-Z][0-9a-zA-Z._/-]*$\n\nHowever, I then realised I should tighten up the R_COMMANDS\nand W_COMMANDS patterns a wee bit; as it stands, if someone\ncould create a file called, say \"git-upload-pack.pwned\", and\nput it in the PATH, he could get the \"git\" user to execute\nit!  Sure if he managed to put something in the PATH that's\nalready game over in some sense but we have to stop what we\ncan :-)  Thanks for catching this.\n\nAnd once again, I really appreciate the extra eyeballs on\nthe code!\n\n-- \nSitaram\n"},{"id":"121722","messageId":"200908251405.17644.jnareb@gmail.com","threadId":"20716","inReplyTo":"2e24e5b90908242253v411ad5f3t8a2802079914d0bf@mail.gmail.com","subject":"Re: gitosis-lite","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2009-08-25T12:05:16Z","receivedAt":"2009-08-25T12:05:16Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Sitaram Chamarty wrote:\n> On Tue, Aug 25, 2009 at 12:14 AM, Jakub Narebski<jnareb@gmail.com> wrote:\n> \n> > A few comments about the code, taking gl-auth-command as example.\n> \n> > Wouldn't it be better to use \"use warnings\" instead of 'perl -w'?\n> \n> I'm not sure what is the minimum perl required for git\n> itself.  Has it needed perl > 5.6.0 for more than a year at\n> least?  The only real difference between these two is scope,\n> which is a non-issue here, so I played safe.\n\nI think that git requires Perl at least version 5.6\n \n> > It would be, I think, better if you have used POD for such\n> > documentation.  One would be able to generate manpage using pod2man,\n> > and it is no less readable in source code.  See e.g. perl/Git.pm or\n> > contrib/hooks/update-paranoid.\n> \n> Hmm... I've been spoiled by Markdown's sane bullet list\n> handling.  Visually, POD forces everything other than code\n> to be flush left -- any sort of list is definitely less\n> readable in source code as a result.  IMHO of course.\n\nHow it is relevant to the issue at hand?  I was talking about replacing\ndocumentation comments in the header with POD markup.\n\nAlso you usually document top-level structures with POD.\n\n\n> > > # first, fix the biggest gripe I have with gitosis, a 1-line change\n> > > my $user=$ENV{GL_USER}=shift;       # there; now that's available everywhere!\n> >\n> > Eh?  This is standalone script, isn't it?  Shouldn't it be\n> >\n> >   my $user = $ENV{GL_USER} = $ARGV[0];       # there; now that's available everywhere!\n> \n> Hmm... I didn't know there was a difference, other than\n> depleting @ARGV, if you're outside a subroutine.  I'll take\n> a relook at it.\n\nIt is, I think, the matter of taste.  IMHO using @ARGV to get _program_\nparameters is better than use 'shift' which is used to get subroutine\narguments.\n\nBTW. have you tried using Perl::Critic or http://perlcritic.com on your\ncode (but remember that those best practice recommendations do not need\nto be followed blindly)?\n\n> > > open(LOG, \">>\", \"$GL_ADMINDIR/log\");\n> > > print LOG \"\\n\", scalar(localtime), \" $ENV{SSH_ORIGINAL_COMMAND} $user\\n\";\n> > > close(LOG);\n> >\n> > It is better practice to use lexical variables instead of barewords\n> > for filehandles:\n> \n> Good catch; thanks!  I guess I'm showing my age :)  Fixed\n> all of them!\n> \n> > Don't forget to check for error.\n> \n> Hmm.. well I'm still debating if a log file write error\n> should block git access / push, but there were two more\n> important closes (again in gl-compile-conf) that were\n> unguarded.  Fixed, thanks.\n\nI was thinking about not writing to log file if you can't open it.\n \n-- \nJakub Narebski\nPoland\n"},{"id":"121770","messageId":"2e24e5b90908252201m66b88ab4gc6ec47f7ad4601c2@mail.gmail.com","threadId":"20716","inReplyTo":"200908251405.17644.jnareb@gmail.com","subject":"Re: gitosis-lite","fromName":"Sitaram Chamarty","fromEmail":"sitaramc@gmail.com","sentAt":"2009-08-26T05:01:21Z","receivedAt":"2009-08-26T05:01:21Z","isPatch":false,"sender":{"key":"sitaramc@gmail.com","avatar":"https://avatars.githubusercontent.com/u/43316?v=4"},"body":"On Tue, Aug 25, 2009 at 5:35 PM, Jakub Narebski<jnareb@gmail.com> wrote:\n> Sitaram Chamarty wrote:\n>> On Tue, Aug 25, 2009 at 12:14 AM, Jakub Narebski<jnareb@gmail.com> wrote:\n\n>> > Wouldn't it be better to use \"use warnings\" instead of 'perl -w'?\n\n>> I'm not sure what is the minimum perl required for git\n\n> I think that git requires Perl at least version 5.6\n\nthanks; I'll change \"-w\" to \"use warnings\"\n\n>> > It would be, I think, better if you have used POD for such\n>> > documentation.  One would be able to generate manpage using pod2man,\n>> > and it is no less readable in source code.  See e.g. perl/Git.pm or\n>> > contrib/hooks/update-paranoid.\n>>\n>> Hmm... I've been spoiled by Markdown's sane bullet list\n>> handling.  Visually, POD forces everything other than code\n>> to be flush left -- any sort of list is definitely less\n>> readable in source code as a result.  IMHO of course.\n>\n> How it is relevant to the issue at hand?  I was talking about replacing\n> documentation comments in the header with POD markup.\n>\n> Also you usually document top-level structures with POD.\n\nForget I mentioned markdown :)  All I'm saying is that even in the\ndocumentation you speak of, I have a couple of small \"lists\".  And I\nlike lists to be properly indented, that's all -- I am mentally unable\nto edit them if they are all flush left :-( just like it's irritating\nto edit code that's mis-indented or not indented.  Sorry...!\n\nAnd yes, one of my python friends has then asked \"why do you not like\npython\".  I have no answer.  As Whitman said\n(http://www.quotationspage.com/quote/26914.html):\n\n    Do I contradict myself?\n    Very well then I contradict myself,\n    (I am large, I contain multitudes.)\n\n:-)\n\nRegards,\n\n-- \nSitaram \"not normally given to quoting poets\" Chamarty\n"}]}