git.vger.kernel.org archive mirror
 help / color / mirror / Atom feed
* gitosis-lite
@ 2009-08-24 12:28 Sitaram Chamarty
  2009-08-24 13:13 ` gitosis-lite Jakub Narebski
  2009-08-24 18:44 ` gitosis-lite Jakub Narebski
  0 siblings, 2 replies; 10+ messages in thread
From: Sitaram Chamarty @ 2009-08-24 12:28 UTC (permalink / raw)
  To: Git Mailing List; +Cc: Tommi Virtanen

[Tommi: I chose this name to reflect that it really owes its existence to
gitosis for the basic ideas etc.  However, if you have any objections I
would be quite happy to find another name.  Just let me know...]



Hello all,

I created a new project called gitosis-lite, which combines
the essential pieces of gitosis with the per-branch
permissions stuff in the example in the howto directory of
git.git.

The config file is different, (there's an annotated example
you can look at).

The "why" and the "what" are all at
http://github.com/sitaramc/gitosis-lite

-- 
Sitaram

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: gitosis-lite
  2009-08-24 12:28 gitosis-lite Sitaram Chamarty
@ 2009-08-24 13:13 ` Jakub Narebski
  2009-08-24 14:35   ` gitosis-lite Sitaram Chamarty
  2009-08-24 18:44 ` gitosis-lite Jakub Narebski
  1 sibling, 1 reply; 10+ messages in thread
From: Jakub Narebski @ 2009-08-24 13:13 UTC (permalink / raw)
  To: Sitaram Chamarty; +Cc: Git Mailing List, Tommi Virtanen

Sitaram Chamarty <sitaramc@gmail.com> writes:

> Hello all,
> 
> I created a new project called gitosis-lite, which combines
> the essential pieces of gitosis with the per-branch
> permissions stuff in the example in the howto directory of
> git.git.
> 
> The config file is different, (there's an annotated example
> you can look at).
> 
> The "why" and the "what" are all at
> http://github.com/sitaramc/gitosis-lite

Could you add information about this tool (perhaps after confirmation
/ deciding on project name[1]) to Git Wiki page
  http://git.or.cz/gitwiki/InterfacesFrontendsAndTools
somewhere below Gitosis?  Please do not forget to include that it is
written in Perl; see other entries for example.


You wrote in project's README.markdown that you were inspired by
Gitosis (which requires Python and python-setuptools) and
Documentation/howto/update-hook-example.txt (which uses bash).
Why not contrib/hooks/update-paranoid (which is written in Perl)?

Using Perl code for configuration is simple and fast, but not very
secure.  Why not use git config format (via "git config -l -z" like in
gitweb), or YAML or JSON (or Storable)?  Well, YAML might be overkill.

BTW. if you blog about gitosis-lite (http://sitaramc.blogspot.com/)
it could be picked up by Perl Iron Man Blogging challenge, and you
could get wider review.

[1]: gitosis-lite doesn't look like CPAN-y name.  Git::Admin perlhaps?
-- 
Jakub Narebski
Poland
ShadeHawk on #git

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: gitosis-lite
  2009-08-24 13:13 ` gitosis-lite Jakub Narebski
@ 2009-08-24 14:35   ` Sitaram Chamarty
  2009-08-24 15:10     ` gitosis-lite Shawn O. Pearce
  2009-08-24 15:43     ` gitosis-lite Jakub Narebski
  0 siblings, 2 replies; 10+ messages in thread
From: Sitaram Chamarty @ 2009-08-24 14:35 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: Git Mailing List, Tommi Virtanen

On Mon, Aug 24, 2009 at 6:43 PM, Jakub Narebski<jnareb@gmail.com> wrote:

> Could you add information about this tool (perhaps after confirmation
> / deciding on project name[1]) to Git Wiki page
>  http://git.or.cz/gitwiki/InterfacesFrontendsAndTools
> somewhere below Gitosis?  Please do not forget to include that it is
> written in Perl; see other entries for example.

Will do; though I'll probably wait until at least one person
other than me has used it :)

> You wrote in project's README.markdown that you were inspired by
> Gitosis (which requires Python and python-setuptools) and
> Documentation/howto/update-hook-example.txt (which uses bash).
> Why not contrib/hooks/update-paranoid (which is written in Perl)?

Hmmm several reasons.  To start with, it appears to me that
update-paranoid assumes each user has his own ACL file, and
I'm going the other way -- you'll notice I have exactly one
file as a single ACL source for many *projects*, leave alone
users.

Secondly, I want to stick to the gitosis philosophy -- it
has served very well, and I'm not sure how "in use"
update-paranoid is.

Thirdly, I'm not comfortable having a hook be so complex.
The whole thing is 10 lines shorter than all my code put
together :)

And finally, it's far too complex for me, leave alone some
of my users.  I count 6 pipe opens in there, and the ACLs
are read by git cat-file or something :) I'll be honest: I
came away feeling very stupid after trying to read and
understand that program.  It was... humbling :(

> Using Perl code for configuration is simple and fast, but not very
> secure.  Why not use git config format (via "git config -l -z" like in

Not secure in what sense?  Only the "git admin" (whoever
owns the account in which gitosis-lite is running) will be
able to generate it, and only scripts that run with his
authority (by way of hooks in repos he owns) can run it.

It cannot do what he did not intend to do, and what he wants
to do he can do anyway, whether it's JSON or perl.

Except for umasking the file, I don't see anything I missed.
Could you help me understand?

> gitweb), or YAML or JSON (or Storable)?  Well, YAML might be overkill.

One of the objectives is to work *as is* on any Unix system
that managed to install git (which implies Perl).  So, no
JSON or YAML.  Storable would be fine (I think it's also
part of core perl) so if I find a compelling reason I don't
mind switching to that.

All I really have is a nested hash to be saved and restored
-- nothing more complex, no objects, no blessed refs, etc.

> BTW. if you blog about gitosis-lite (http://sitaramc.blogspot.com/)
> it could be picked up by Perl Iron Man Blogging challenge, and you
> could get wider review.

Will do; thanks.  I did not know my blog got picked up by
anything; till date it has never come up on a google search
or a blog search anywhere, and it's more of a rant-box +
annotated bookmarking service for myself :)

> [1]: gitosis-lite doesn't look like CPAN-y name.  Git::Admin perlhaps?

Too presumptuous for someone like me -- makes it sound like
the "one true way to Admin git" :)

Thank you very much for your comments!

-- 
Sitaram

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: gitosis-lite
  2009-08-24 14:35   ` gitosis-lite Sitaram Chamarty
@ 2009-08-24 15:10     ` Shawn O. Pearce
  2009-08-25  3:00       ` gitosis-lite Sitaram Chamarty
  2009-08-24 15:43     ` gitosis-lite Jakub Narebski
  1 sibling, 1 reply; 10+ messages in thread
From: Shawn O. Pearce @ 2009-08-24 15:10 UTC (permalink / raw)
  To: Sitaram Chamarty; +Cc: Jakub Narebski, Git Mailing List, Tommi Virtanen

Sitaram Chamarty <sitaramc@gmail.com> wrote:
> On Mon, Aug 24, 2009 at 6:43 PM, Jakub Narebski<jnareb@gmail.com> wrote:
> > Why not contrib/hooks/update-paranoid (which is written in Perl)?
> 
> Hmmm several reasons.  To start with, it appears to me that
> update-paranoid assumes each user has his own ACL file, and
> I'm going the other way -- you'll notice I have exactly one
> file as a single ACL source for many *projects*, leave alone
> users.

That is true, update-paranoid uses an ACL file per user, because
we approached it from the perspective of "what can this user do"
rather than "what is allowed in this project".

We later ran into needing groups, which meant that in practice the
per-user ACL files really just enumerated what groups they were
a member of, and the group ACL files enumerated what they were
allowed to do in projects.

I can certainly see how this might be backwards, and someone would
prefer the other direction, a per-project listing or something.
 
> Secondly, I want to stick to the gitosis philosophy -- it
> has served very well, and I'm not sure how "in use"
> update-paranoid is.

Well, my prior job used it heavily, and I think is still using it,
a year after I left.  My current job would use it, except that we
went and wrote Gerrit Code Review instead.  :-)
 
> Thirdly, I'm not comfortable having a hook be so complex.
> The whole thing is 10 lines shorter than all my code put
> together :)

Yes, update-paranoid is a big script.  It does a lot.  It not only
checks "can you write in this repository, on this branch" but it
also checks that every commit matches your own email address(es),
and it can also check file path edits, to see if you can modify
the files that were changed.
 
> And finally, it's far too complex for me, leave alone some
> of my users.  I count 6 pipe opens in there,

Not surprised, it needs to read a lot of data from git to do its
job correctly.  It has to enumerate all new commits, any commits
which may be discarded by the update (non-fast-forward), as well
as scan the diff to see which paths were affected.

> and the ACLs
> are read by git cat-file or something

Because I didn't want to check out a copy of the ACLs to the local
disk just to read them into the hook.  The hook pulls them directly
out of a bare "admin" repository, making it easier to push changes
into the repository and have them take effect immediately.

I know Gitosis handles this by having a magic hook in the admin
repository that copies the file out.  update-paranoid just reads
it from git.

> I'll be honest: I
> came away feeling very stupid after trying to read and
> understand that program.  It was... humbling :(

*sigh* That's not good, the hook is meant as a practical example,
if it was too complex to understand, I did a poor job of writing it.
 
-- 
Shawn.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: gitosis-lite
  2009-08-24 14:35   ` gitosis-lite Sitaram Chamarty
  2009-08-24 15:10     ` gitosis-lite Shawn O. Pearce
@ 2009-08-24 15:43     ` Jakub Narebski
  1 sibling, 0 replies; 10+ messages in thread
From: Jakub Narebski @ 2009-08-24 15:43 UTC (permalink / raw)
  To: Sitaram Chamarty; +Cc: Git Mailing List, Tommi Virtanen

On Mon, 24 August 2009, Sitaram Chamarty wrote:
> On Mon, Aug 24, 2009 at 6:43 PM, Jakub Narebski<jnareb@gmail.com> wrote:
> 
> > Could you add information about this tool (perhaps after confirmation
> > / deciding on project name[1]) to Git Wiki page
> >  http://git.or.cz/gitwiki/InterfacesFrontendsAndTools
> > somewhere below Gitosis?  Please do not forget to include that it is
> > written in Perl; see other entries for example.
> 
> Will do; though I'll probably wait until at least one person
> other than me has used it :)

On the other hand having the link in IFAT wiki page could help you
gain more users of gitosis-lite (name pending ;-)

> > You wrote in project's README.markdown that you were inspired by
> > Gitosis (which requires Python and python-setuptools) and
> > Documentation/howto/update-hook-example.txt (which uses bash).
> > Why not contrib/hooks/update-paranoid (which is written in Perl)?
> 
> Hmmm several reasons.  To start with, it appears to me that
> update-paranoid assumes each user has his own ACL file, and
> I'm going the other way -- you'll notice I have exactly one
> file as a single ACL source for many *projects*, leave alone
> users.

Well, gitosis uses single SSH user: 'git' (by default, at least).

Also from what I understand update-paranoid uses the same set of ACLs
for all projects (well, depending how $acl_git and $acl_branch are
defined).
 
> Secondly, I want to stick to the gitosis philosophy -- it
> has served very well, and I'm not sure how "in use"
> update-paranoid is.

I wonder about this too.
 
> Thirdly, I'm not comfortable having a hook be so complex.
> The whole thing is 10 lines shorter than all my code put
> together :)

The update-paranoid hook has inline POD documentation, which adds to
its size (I think that your gitosis-lite (name pending) should also
use this Perl literate programming convention).  On the other hand
this documentation is not, unfortunately, up to date; for example
it allows to control which directories you can change, which is not
documented in POD.

> And finally, it's far too complex for me, leave alone some
> of my users.  I count 6 pipe opens in there, and the ACLs
> are read by git cat-file or something :)

That's because update-paranoid, like gitosis, uses configuration in
$scl_repo repository.  git-cat-file is low level (plumbing) equivalent
of git-show (porcelain).

Those pipelines are required for strict (paranoid) access control that
update-paranoid uses: checking that committers are on allowed list,
that taggers are on allowed list, that branches points to commits, that
tags points to tag objects, etc.

> I'll be honest: I 
> came away feeling very stupid after trying to read and
> understand that program.  It was... humbling :(

On the other hand it uses subroutine prototypes (unnecessary, and not
recommended), old-style bareword filehandles instead of lexically scoped
filehandles ("open I, ..." vs "open my $fh, ...")
 
> > Using Perl code for configuration is simple and fast, but not very
> > secure.  Why not use git config format (via "git config -l -z" like in
> 
> Not secure in what sense?  Only the "git admin" (whoever
> owns the account in which gitosis-lite is running) will be
> able to generate it, and only scripts that run with his
> authority (by way of hooks in repos he owns) can run it.
> 
> It cannot do what he did not intend to do, and what he wants
> to do he can do anyway, whether it's JSON or perl.
> 
> Except for umasking the file, I don't see anything I missed.
> Could you help me understand?

Ah, thats O.K. then.

> 
> > gitweb), or YAML or JSON (or Storable)?  Well, YAML might be overkill.
> 
> One of the objectives is to work *as is* on any Unix system
> that managed to install git (which implies Perl).  So, no
> JSON or YAML.  Storable would be fine (I think it's also
> part of core perl) so if I find a compelling reason I don't
> mind switching to that.

One reason to not use Storable is that it is binary.  Another that format
is not exactly stable (but it contains version info, so it is backwards
compatibile I think).

As for dependencies: some people on #perl told me to not worry about
dependencies because there is PAR :-)

> 
> All I really have is a nested hash to be saved and restored
> -- nothing more complex, no objects, no blessed refs, etc.

JSON would be good then ;-)

> 
> > BTW. if you blog about gitosis-lite (http://sitaramc.blogspot.com/)
> > it could be picked up by Perl Iron Man Blogging challenge, and you
> > could get wider review.
> 
> Will do; thanks.  I did not know my blog got picked up by
> anything; till date it has never come up on a google search
> or a blog search anywhere, and it's more of a rant-box +
> annotated bookmarking service for myself :)

I don't know if it would be picked; still, you can try to submit it to
(join) http://ironman.enlightenedperl.org; note that only posts which
contain one of magic words ('perl', 'ironman', ...) would get picked by
this planet.

> 
> > [1]: gitosis-lite doesn't look like CPAN-y name.  Git::Admin perlhaps?
> 
> Too presumptuous for someone like me -- makes it sound like
> the "one true way to Admin git" :)

Git::Auth perhaps (authentication and authorization)?  
Git::AccessControl?

You can always ask on #perl (also on freenode).
-- 
Jakub Narebski
Poland

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: gitosis-lite
  2009-08-24 12:28 gitosis-lite Sitaram Chamarty
  2009-08-24 13:13 ` gitosis-lite Jakub Narebski
@ 2009-08-24 18:44 ` Jakub Narebski
  2009-08-25  5:53   ` gitosis-lite Sitaram Chamarty
  1 sibling, 1 reply; 10+ messages in thread
From: Jakub Narebski @ 2009-08-24 18:44 UTC (permalink / raw)
  To: Sitaram Chamarty; +Cc: Git Mailing List, Tommi Virtanen

Sitaram Chamarty <sitaramc@gmail.com> writes:

> I created a new project called gitosis-lite, which combines
> the essential pieces of gitosis with the per-branch
> permissions stuff in the example in the howto directory of
> git.git.

As for the name: gitness, gitamine, gitrify,... ;-)
 
> The config file is different, (there's an annotated example
> you can look at).
> 
> The "why" and the "what" are all at
> http://github.com/sitaramc/gitosis-lite

A few comments about the code, taking gl-auth-command as example.

> #!/usr/bin/perl -w
>
> use strict;

Wouldn't it be better to use "use warnings" instead of 'perl -w'?

> # === auth-command ===
> # the command that GL users actually run
> 
> # part of the gitosis-lite (GL) suite
> 
> # how run:      via sshd, being listed in "command=" in ssh authkeys
> # when:         every login by a GL user
> # input:        $1 is GL username, plus $SSH_ORIGINAL_COMMAND
> # output:
> # security:
> #     - currently, we just make some basic checks, copied from gitosis
>
> # robustness:
>
> # other notes:

It would be, I think, better if you have used POD for such
documentation.  One would be able to generate manpage using pod2man,
and it is no less readable in source code.  See e.g. perl/Git.pm or
contrib/hooks/update-paranoid.

> our $GL_ADMINDIR;
> our $GL_CONF;
> our $GL_KEYDIR;
> our $GL_CONF_COMPILED;
> our $REPO_BASE;
> our %repos;

Why is the reason behind using 'our' instead of 'my' here?

> # first, fix the biggest gripe I have with gitosis, a 1-line change
> my $user=$ENV{GL_USER}=shift;       # there; now that's available everywhere!

Eh?  This is standalone script, isn't it?  Shouldn't it be

  my $user = $ENV{GL_USER} = $ARGV[0];       # there; now that's available everywhere!

> my $perm = 'W'; $perm = 'R' if $verb =~ $R_COMMANDS;

Either split it into two lines, or use ?: confitional operator:

  my $perm = ($verb =~ $R_COMMANDS ? 'R' : 'W');

> open(LOG, ">>", "$GL_ADMINDIR/log");
> print LOG "\n", scalar(localtime), " $ENV{SSH_ORIGINAL_COMMAND} $user\n";
> close(LOG);

It is better practice to use lexical variables instead of barewords
for filehandles:

  if (open my $logfh, ">>", "$GL_ADMINDIR/log") {
  	print $logfh "\n", scalar(localtime), " $ENV{SSH_ORIGINAL_COMMAND} $user\n";
  	close $logfh;
  }

Don't forget to check for error.

> $repo = "'$REPO_BASE/$repo.git'";
> exec("git", "shell", "-c", "$verb $repo");

That's not enough.  You have to shell-quote $repo, like in gitweb or
using String::ShellQuote module, or somehow use list form to pass
arguments to git-shell.  You protect here againts spaces in filename,
but not againts "'" (single quote) and for show shells "!"
(exclamation mark).

-- 
Jakub Narebski
Poland
ShadeHawk on #git

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: gitosis-lite
  2009-08-24 15:10     ` gitosis-lite Shawn O. Pearce
@ 2009-08-25  3:00       ` Sitaram Chamarty
  0 siblings, 0 replies; 10+ messages in thread
From: Sitaram Chamarty @ 2009-08-25  3:00 UTC (permalink / raw)
  To: Shawn O. Pearce; +Cc: Jakub Narebski, Git Mailing List, Tommi Virtanen

On Mon, Aug 24, 2009 at 8:40 PM, Shawn O. Pearce<spearce@spearce.org> wrote:
> Sitaram Chamarty <sitaramc@gmail.com> wrote:

>> I'll be honest: I
>> came away feeling very stupid after trying to read and
>> understand that program.  It was... humbling :(

> *sigh* That's not good, the hook is meant as a practical example,
> if it was too complex to understand, I did a poor job of writing it.

Hmmm no.  It was just doing too much to grasp in one
reading, especially by someone whose perl seems to have
rusted a wee bit, if you've seen Jakub's reply :)

I took a slightly longer look after your mail and I grokked
it a lot better.  And wow... I guess you called it
"paranoid" for a reason :-)

In the environments I'm catering to, every commit will be
reviewed by someone else (usually someone more experienced,
and having a bigger picture), so "you're not allowed to
touch this file" type of thing will all come out.

All I need is to make sure the important branches can only
be pushed to by certain people; the "process" will take care
of the rest.

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: gitosis-lite
  2009-08-24 18:44 ` gitosis-lite Jakub Narebski
@ 2009-08-25  5:53   ` Sitaram Chamarty
  2009-08-25 12:05     ` gitosis-lite Jakub Narebski
  0 siblings, 1 reply; 10+ messages in thread
From: Sitaram Chamarty @ 2009-08-25  5:53 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: Git Mailing List

[took Tommi out of cc; he must be getting enough mail as is...]

On Tue, Aug 25, 2009 at 12:14 AM, Jakub Narebski<jnareb@gmail.com> wrote:

> A few comments about the code, taking gl-auth-command as example.

> Wouldn't it be better to use "use warnings" instead of 'perl -w'?

I'm not sure what is the minimum perl required for git
itself.  Has it needed perl > 5.6.0 for more than a year at
least?  The only real difference between these two is scope,
which is a non-issue here, so I played safe.

> It would be, I think, better if you have used POD for such
> documentation.  One would be able to generate manpage using pod2man,
> and it is no less readable in source code.  See e.g. perl/Git.pm or
> contrib/hooks/update-paranoid.

Hmm... I've been spoiled by Markdown's sane bullet list
handling.  Visually, POD forces everything other than code
to be flush left -- any sort of list is definitely less
readable in source code as a result.  IMHO of course.

>> our $GL_ADMINDIR;
>> our $GL_CONF;
>> our $GL_KEYDIR;
>> our $GL_CONF_COMPILED;
>> our $REPO_BASE;
>> our %repos;
>
> Why is the reason behind using 'our' instead of 'my' here?

They are assigned values in some file being "do"-ed, so they
can't be lexical scoped.  However, I found a few others that
were holdovers from an earlier version.  Fixed; thanks for
catching that.

>> # first, fix the biggest gripe I have with gitosis, a 1-line change
>> my $user=$ENV{GL_USER}=shift;       # there; now that's available everywhere!
>
> Eh?  This is standalone script, isn't it?  Shouldn't it be
>
>   my $user = $ENV{GL_USER} = $ARGV[0];       # there; now that's available everywhere!

Hmm... I didn't know there was a difference, other than
depleting @ARGV, if you're outside a subroutine.  I'll take
a relook at it.

>> my $perm = 'W'; $perm = 'R' if $verb =~ $R_COMMANDS;
>
> Either split it into two lines, or use ?: confitional operator:
>
>   my $perm = ($verb =~ $R_COMMANDS ? 'R' : 'W');

much nicer... Fixed, thanks.

>> open(LOG, ">>", "$GL_ADMINDIR/log");
>> print LOG "\n", scalar(localtime), " $ENV{SSH_ORIGINAL_COMMAND} $user\n";
>> close(LOG);
>
> It is better practice to use lexical variables instead of barewords
> for filehandles:

Good catch; thanks!  I guess I'm showing my age :)  Fixed
all of them!

> Don't forget to check for error.

Hmm.. well I'm still debating if a log file write error
should block git access / push, but there were two more
important closes (again in gl-compile-conf) that were
unguarded.  Fixed, thanks.

>> $repo = "'$REPO_BASE/$repo.git'";
>> exec("git", "shell", "-c", "$verb $repo");
>
> That's not enough.  You have to shell-quote $repo, like in gitweb or
> using String::ShellQuote module, or somehow use list form to pass
> arguments to git-shell.  You protect here againts spaces in filename,
> but not againts "'" (single quote) and for show shells "!"
> (exclamation mark).

It'll never get here.  It'll die much earlier if the
reponame does not match a much stricter pattern.  Maybe too
strict, actually: ^[0-9a-zA-Z][0-9a-zA-Z._/-]*$

However, I then realised I should tighten up the R_COMMANDS
and W_COMMANDS patterns a wee bit; as it stands, if someone
could create a file called, say "git-upload-pack.pwned", and
put it in the PATH, he could get the "git" user to execute
it!  Sure if he managed to put something in the PATH that's
already game over in some sense but we have to stop what we
can :-)  Thanks for catching this.

And once again, I really appreciate the extra eyeballs on
the code!

-- 
Sitaram

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: gitosis-lite
  2009-08-25  5:53   ` gitosis-lite Sitaram Chamarty
@ 2009-08-25 12:05     ` Jakub Narebski
  2009-08-26  5:01       ` gitosis-lite Sitaram Chamarty
  0 siblings, 1 reply; 10+ messages in thread
From: Jakub Narebski @ 2009-08-25 12:05 UTC (permalink / raw)
  To: Sitaram Chamarty; +Cc: Git Mailing List

Sitaram Chamarty wrote:
> On Tue, Aug 25, 2009 at 12:14 AM, Jakub Narebski<jnareb@gmail.com> wrote:
> 
> > A few comments about the code, taking gl-auth-command as example.
> 
> > Wouldn't it be better to use "use warnings" instead of 'perl -w'?
> 
> I'm not sure what is the minimum perl required for git
> itself.  Has it needed perl > 5.6.0 for more than a year at
> least?  The only real difference between these two is scope,
> which is a non-issue here, so I played safe.

I think that git requires Perl at least version 5.6
 
> > It would be, I think, better if you have used POD for such
> > documentation.  One would be able to generate manpage using pod2man,
> > and it is no less readable in source code.  See e.g. perl/Git.pm or
> > contrib/hooks/update-paranoid.
> 
> Hmm... I've been spoiled by Markdown's sane bullet list
> handling.  Visually, POD forces everything other than code
> to be flush left -- any sort of list is definitely less
> readable in source code as a result.  IMHO of course.

How it is relevant to the issue at hand?  I was talking about replacing
documentation comments in the header with POD markup.

Also you usually document top-level structures with POD.


> > > # first, fix the biggest gripe I have with gitosis, a 1-line change
> > > my $user=$ENV{GL_USER}=shift;       # there; now that's available everywhere!
> >
> > Eh?  This is standalone script, isn't it?  Shouldn't it be
> >
> >   my $user = $ENV{GL_USER} = $ARGV[0];       # there; now that's available everywhere!
> 
> Hmm... I didn't know there was a difference, other than
> depleting @ARGV, if you're outside a subroutine.  I'll take
> a relook at it.

It is, I think, the matter of taste.  IMHO using @ARGV to get _program_
parameters is better than use 'shift' which is used to get subroutine
arguments.

BTW. have you tried using Perl::Critic or http://perlcritic.com on your
code (but remember that those best practice recommendations do not need
to be followed blindly)?

> > > open(LOG, ">>", "$GL_ADMINDIR/log");
> > > print LOG "\n", scalar(localtime), " $ENV{SSH_ORIGINAL_COMMAND} $user\n";
> > > close(LOG);
> >
> > It is better practice to use lexical variables instead of barewords
> > for filehandles:
> 
> Good catch; thanks!  I guess I'm showing my age :)  Fixed
> all of them!
> 
> > Don't forget to check for error.
> 
> Hmm.. well I'm still debating if a log file write error
> should block git access / push, but there were two more
> important closes (again in gl-compile-conf) that were
> unguarded.  Fixed, thanks.

I was thinking about not writing to log file if you can't open it.
 
-- 
Jakub Narebski
Poland

^ permalink raw reply	[flat|nested] 10+ messages in thread

* Re: gitosis-lite
  2009-08-25 12:05     ` gitosis-lite Jakub Narebski
@ 2009-08-26  5:01       ` Sitaram Chamarty
  0 siblings, 0 replies; 10+ messages in thread
From: Sitaram Chamarty @ 2009-08-26  5:01 UTC (permalink / raw)
  To: Jakub Narebski; +Cc: Git Mailing List

On Tue, Aug 25, 2009 at 5:35 PM, Jakub Narebski<jnareb@gmail.com> wrote:
> Sitaram Chamarty wrote:
>> On Tue, Aug 25, 2009 at 12:14 AM, Jakub Narebski<jnareb@gmail.com> wrote:

>> > Wouldn't it be better to use "use warnings" instead of 'perl -w'?

>> I'm not sure what is the minimum perl required for git

> I think that git requires Perl at least version 5.6

thanks; I'll change "-w" to "use warnings"

>> > It would be, I think, better if you have used POD for such
>> > documentation.  One would be able to generate manpage using pod2man,
>> > and it is no less readable in source code.  See e.g. perl/Git.pm or
>> > contrib/hooks/update-paranoid.
>>
>> Hmm... I've been spoiled by Markdown's sane bullet list
>> handling.  Visually, POD forces everything other than code
>> to be flush left -- any sort of list is definitely less
>> readable in source code as a result.  IMHO of course.
>
> How it is relevant to the issue at hand?  I was talking about replacing
> documentation comments in the header with POD markup.
>
> Also you usually document top-level structures with POD.

Forget I mentioned markdown :)  All I'm saying is that even in the
documentation you speak of, I have a couple of small "lists".  And I
like lists to be properly indented, that's all -- I am mentally unable
to edit them if they are all flush left :-( just like it's irritating
to edit code that's mis-indented or not indented.  Sorry...!

And yes, one of my python friends has then asked "why do you not like
python".  I have no answer.  As Whitman said
(http://www.quotationspage.com/quote/26914.html):

    Do I contradict myself?
    Very well then I contradict myself,
    (I am large, I contain multitudes.)

:-)

Regards,

-- 
Sitaram "not normally given to quoting poets" Chamarty

^ permalink raw reply	[flat|nested] 10+ messages in thread

end of thread, other threads:[~2009-08-26  5:01 UTC | newest]

Thread overview: 10+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2009-08-24 12:28 gitosis-lite Sitaram Chamarty
2009-08-24 13:13 ` gitosis-lite Jakub Narebski
2009-08-24 14:35   ` gitosis-lite Sitaram Chamarty
2009-08-24 15:10     ` gitosis-lite Shawn O. Pearce
2009-08-25  3:00       ` gitosis-lite Sitaram Chamarty
2009-08-24 15:43     ` gitosis-lite Jakub Narebski
2009-08-24 18:44 ` gitosis-lite Jakub Narebski
2009-08-25  5:53   ` gitosis-lite Sitaram Chamarty
2009-08-25 12:05     ` gitosis-lite Jakub Narebski
2009-08-26  5:01       ` gitosis-lite Sitaram Chamarty

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).