gnustavo / Git-Hooks

Framework for implementing Git (and Gerrit) hooks
http://search.cpan.org/dist/Git-Hooks/
41 stars 17 forks source link

Add a way to access githooks.userenv value #64

Closed mikkoi closed 4 years ago

mikkoi commented 4 years ago

Config item githooks.userenv is used to name an environmental variable which provides the authenticated user name. This is normally USER when accessing repo via SSH or REMOTE_USER when using a webserver. When it is not possible to just name an environmental variable, the value can be generated by a perl sentence. In such a case, the value should be available for use in other configurations, e.g. CheckReference acls.

This change is more controversial. I tried to pick a name that would never clash with a real environmental variable. Of course, the name can be changed.

Signed-off-by: Mikko Koivunalho mikko.koivunalho@iki.fi

gnustavo commented 4 years ago

Great idea, Mikko. Thanks!

I'll make a few suggestions on the code itself.

gnustavo commented 4 years ago

Mikko, I think it would be easier and more general if we simply exposed the authenticated user name in an environment variable. I mean, Git::Hooks could set an environment variable called, for example, GITHOOKS_AUTHENTICATED_USER. Then it would be available as is in the acl specifications and in other places were environment variables are used.

What do you think?

mikkoi commented 4 years ago

Do you mean like this:

diff --git a/lib/Git/Repository/Plugin/GitHooks.pm b/lib/Git/Repository/Plugin/GitHooks.pm
index 755db42..7f77820 100644
--- a/lib/Git/Repository/Plugin/GitHooks.pm
+++ b/lib/Git/Repository/Plugin/GitHooks.pm
@@ -1260,6 +1260,7 @@ sub authenticated_user {
         } else {
             $git->{_plugin_githooks}{authenticated_user} = $ENV{GERRIT_USER_EMAIL} || $ENV{USER} || undef;
         }
+        $ENV{GITHOOKS_AUTHENTICATED_USER} = $git->{_plugin_githooks}{authenticated_user};
     }

     return $git->{_plugin_githooks}{authenticated_user};

I think it would work even better. It is much better.

gnustavo commented 4 years ago

Yes, like that.

I'm going to make a new release with this change soon.

mikkoi commented 4 years ago

Great @gnustavo . Thanks. Much smaller change than the one I proposed in the beginning. We narrowed it down! :-)

But I think there might be a problem in the commit you made.

local $ENV{GITHOOKS_AUTHENTICATED_USER} = $git->authenticated_user();

If you use local, the change will not affect the process wide %ENV variable. See https://pastebin.pl/view/b1e4c4f2

gnustavo commented 4 years ago

The local introduces a dynamic scope and not a static scope, as does my. In this case it will change the environment variable from the point where it's set until the end of the run_hook function, making it available for all functions called by run_hook.

I changed your example a bit to make it clearer: https://pastebin.pl/view/51301efc

gnustavo commented 4 years ago

BTW, I use local in order to placate Perl::Critic's RequireLocalizedPunctuationVars policy. I could make Perl::Critic disregard this particular assignment but in this case it doesn't matter and I complied.

mikkoi commented 4 years ago

Damn Perl::Critic! Yes, it does complain, of course. For once, it is perfectly legal to put that

## no critic (Variables::RequireLocalizedPunctuationVars)

:grinning:

mikkoi commented 4 years ago

Implemented in a different way in https://github.com/gnustavo/Git-Hooks/commit/7e21bf3fdcef242f6a875668aeb57b50551f712a