Re: [PATCH] git-cvsserver: run post-update hook *after* update.

Stefan Karpinski [off-list ref] writes:

I know that this and the other patch I sent are completely trivial and
uninteresting, but they would appear to be correct. Do I need to prod
more to get them included or what? Did I submit them incorrectly?

If you spend the bandwidth to quote the whole patch, don't quote it, but
please use the same bandwidth to resend it --- that way, if the reason
your patch left unapplied was because your earlier submission was lost in
the noise or too heavy maintainer workload, it can be easily picked up.

Upon my cursory look the patch looks sane, even though it risks breaking
people's scripts that relied on the incorrect behaviour of running the
hook before the update is done, which is slightly worrysome.  Find out who
are knowledgeable in the area of the code you are touching, and Cc them to
ask their input.  "git shortlog -s -n git-cvsserver.perl" may help.

Please sign your patch.

Thanks.

Oh, and one more thing.  Please do not top post.

On Fri, Jan 16, 2009 at 2:22 PM, Stefan Karpinski
[off-list ref] wrote:

quoted

CVS server was running the hook before the update
action was actually done. This performs the update
before the hook is called.
---

Unless I'm severely misunderstanding the meaning of
a *post-update* hook, I think this is a no-brainer.

 git-cvsserver.perl |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index c1e09ea..d2e6003 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl

@@ -1413,14 +1413,14 @@ sub req_ci
               close $pipe || die "bad pipe: $! $?";
       }

+    $updater->update();
+
       ### Then hooks/post-update
       $hook = $ENV{GIT_DIR}.'hooks/post-update';
       if (-x $hook) {
               system($hook, "refs/heads/$state->{module}");
       }

-    $updater->update();
-
    # foreach file specified on the command line ...
    foreach my $filename ( @committedfiles )
    {
--

1.6.0.3.3.g08dd8

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

CVS server was running the hook before the update action was
actually done. This performs the update before the hook is called.

The original commit that introduced the current incorrect behavior
was 394d66d "git-cvsserver runs hooks/post-update". The error in
ordering of the hook call appears to have gone unnoticed, but since
git-cvsserver is supposed to emulate receive-pack, it stands to
reason that the hook should be run *after* the update. Since this
behavior is inconsistent with recieve-pack, users are either:

  1) not using post-update hooks with git-cvsserver;
  2) using post-update hooks that don't care whether they are
     called before or after the actual update occurs;
  3) using post-update hooks *only* with git-cvsserver, and
     relying on the hook being called just before the update.

This patch would affect only users in case 3. These users are
depending on fairly obviously wrong behavior, and moreover they can
simply change their current post-update into post-recieve hooks,
and their systems will work correctly again.

Signed-off-by: Stefan Karpinski <redacted>
---
I'm CCing Andy Parkins, Michael Witten, and Junio Hamano, who
authored the other three commits implementing or affecting hooks in
git-cvsserver (394d66d, cdf6328, b2741f6). If you could please take
a look at this patch and comment on if it's harmful or not, it
would be much appreciated.

 git-cvsserver.perl |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index c1e09ea..d2e6003 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl

@@ -1413,14 +1413,14 @@ sub req_ci
 		close $pipe || die "bad pipe: $! $?";
 	}
 
+    $updater->update();
+
 	### Then hooks/post-update
 	$hook = $ENV{GIT_DIR}.'hooks/post-update';
 	if (-x $hook) {
 		system($hook, "refs/heads/$state->{module}");
 	}
 
-    $updater->update();
-
     # foreach file specified on the command line ...
     foreach my $filename ( @committedfiles )
     {

-- 
1.6.0.3.3.g08dd8

On Thursday 29 January 2009 21:58:02 Stefan Karpinski wrote:

This patch would affect only users in case 3. These users are
depending on fairly obviously wrong behavior, and moreover they can
simply change their current post-update into post-recieve hooks,
and their systems will work correctly again.

Quite right.

Signed-off-by: Stefan Karpinski <redacted>

Acked-By: Andy Parkins <redacted>

-- 
Dr Andy Parkins
andyparkins@gmail.com