threads / patch / 17318

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

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

## tl;dr

12 messages between Jan 23, 2009 and Jan 30, 2009. Diffs are folded; open one to read it.

replies: 11people: 4as markdown or json

Stefan Karpinski· Jan 23, 2009, 05:43 UTC · lore

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?

On Fri, Jan 16, 2009 at 2:22 PM, Stefan Karpinski <stefan.karpinski@gmail.com> wrote:

Show 36 quoted lines
>
> 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
>
Junio C Hamano· Jan 23, 2009, 08:00 UTC · re: Stefan Karpinski · lore
Stefan Karpinski <stefan.karpinski@gmail.com> 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.
Show 42 quoted lines
> On Fri, Jan 16, 2009 at 2:22 PM, Stefan Karpinski
> <stefan.karpinski@gmail.com> wrote:
>>
>> 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
Stefan Karpinski· Jan 29, 2009, 21:58 UTC · re: Junio C Hamano · lore

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

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 <stefan.karpinski@gmail.com>
---
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(-)
Show changes to git-cvsserver.perl +2 −2
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
Junio C Hamano· Jan 29, 2009, 22:48 UTC · re: Stefan Karpinski · lore
Stefan Karpinski <stefan.karpinski@gmail.com> writes:
Show 28 quoted lines
> 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 <stefan.karpinski@gmail.com>
> ---
> 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.

I think I've seen this one before and I thought it was a sensible thing to do (and perhaps I even said so here).

Is this a resend? If so, let's queue it in at least 'next' and see if anybody screams ;-). For a program near the fringe like cvsserver, not many people run it but the small number of people who run it gets hurt rather quickly if the updated behaviour breaks their existing practice, and sometimes breaking things for them would be the only way to extract any response. Yes, it is very unfortunate.

Show 26 quoted lines
>  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
Stefan Karpinski· Jan 29, 2009, 23:26 UTC · re: Junio C Hamano · lore
On Thu, Jan 29, 2009 at 2:48 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> I think I've seen this one before and I thought it was a sensible thing to
> do (and perhaps I even said so here).
You said it looked sane but that I should resend CCing knowledgable parties.
Show 6 quoted lines
> Is this a resend?  If so, let's queue it in at least 'next' and see if
> anybody screams ;-).  For a program near the fringe like cvsserver, not
> many people run it but the small number of people who run it gets hurt
> rather quickly if the updated behaviour breaks their existing practice,
> and sometimes breaking things for them would be the only way to extract
> any response.  Yes, it is very unfortunate.

Yes, it is a resend, but I expanded on the commit message, including my analysis of the potential impact.

Andy Parkins· Jan 29, 2009, 22:56 UTC · re: Stefan Karpinski · lore
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 <stefan.karpinski@gmail.com>
Acked-By: Andy Parkins <andyparkins@gmail.com>
-- 
Dr Andy Parkins
andyparkins@gmail.com
Stefan Karpinski· Jan 29, 2009, 21:35 UTC · lore

[PATCH] git-cvsserver: handle CVS 'noop' command.

The implementation is trivial: ignore the 'noop' command if it is sent. This command is issued by some CVS clients, notably TortoiseCVS. Without this patch, TortoiseCVS will choke when git-cvsserver complains about the unsupported command.

Signed-off-by: Stefan Karpinski <stefan.karpinski@gmail.com>
---

Since this change has no negative impact, is too simple to be wrong, and improves interaction with some clients, it seem to me like a no-brainer to apply it.

 git-cvsserver.perl |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)
Show changes to git-cvsserver.perl +1 −1
diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index fef7faf..c1e09ea 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -188,7 +188,7 @@ while (<STDIN>)
         # use the $methods hash to call the appropriate sub for this command
         #$log->info("Method : $1");
         &{$methods->{$1}}($1,$2);
-    } else {
+    } elsif ($1 ne 'noop') {
         # log fatal because we don't understand this function. If this happens
         # we're fairly screwed because we don't know if the client is expecting
         # a response. If it is, the client will hang, we'll hang, and the whole

-- 
1.6.0.3.3.g08dd8
Junio C Hamano· Jan 29, 2009, 22:45 UTC · re: Stefan Karpinski · lore

Re: [PATCH] git-cvsserver: handle CVS 'noop' command.

Stefan Karpinski <stefan.karpinski@gmail.com> writes:
Show 31 quoted lines
> The implementation is trivial: ignore the 'noop' command
> if it is sent. This command is issued by some CVS clients,
> notably TortoiseCVS. Without this patch, TortoiseCVS will
> choke when git-cvsserver complains about the unsupported
> command.
>
> Signed-off-by: Stefan Karpinski <stefan.karpinski@gmail.com>
> ---
>
> Since this change has no negative impact, is too simple to
> be wrong, and improves interaction with some clients, it
> seem to me like a no-brainer to apply it.
>
>  git-cvsserver.perl |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/git-cvsserver.perl b/git-cvsserver.perl
> index fef7faf..c1e09ea 100755
> --- a/git-cvsserver.perl
> +++ b/git-cvsserver.perl
> @@ -188,7 +188,7 @@ while (<STDIN>)
>          # use the $methods hash to call the appropriate sub for this command
>          #$log->info("Method : $1");
>          &{$methods->{$1}}($1,$2);
> -    } else {
> +    } elsif ($1 ne 'noop') {
>          # log fatal because we don't understand this function. If this happens
>          # we're fairly screwed because we don't know if the client is expecting
>          # a response. If it is, the client will hang, we'll hang, and the whole
> -- 
> 1.6.0.3.3.g08dd8
Not a no-brainer at all, sorry.

Imagine what you would do when you discover another request a random other client sends that you would want to ignore just like you did for 'noop'. Viewed in this light, your patch is a very short sighted one that has a big negative impact on maintainability.

A true no-brainer that has no negative impact would have been something like the attached patch, that adds a method that does not do anything.

Even then, between req_CATCHALL and req_EMPTY, I am not sure which one is expected by the clients, without consulting to the protocol documentation for cvs server/client communication. In the attached patch, I am guessing from your patch that at least Tortoise does not expect any response to it.

 git-cvsserver.perl |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
Show changes to diff +1 −0
diff --git i/git-cvsserver.perl w/git-cvsserver.perl
index fef7faf..ca47e08 100755
--- i/git-cvsserver.perl
+++ w/git-cvsserver.perl
@@ -71,6 +71,7 @@ my $methods = {
     'log'             => \&req_log,
     'rlog'            => \&req_log,
     'tag'             => \&req_CATCHALL,
+    'noop'            => \&req_CATCHALL,
     'status'          => \&req_status,
     'admin'           => \&req_CATCHALL,
     'history'         => \&req_CATCHALL,
Stefan Karpinski· Jan 29, 2009, 23:39 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-cvsserver: handle CVS 'noop' command.

On Thu, Jan 29, 2009 at 2:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 5 quoted lines
>
> Imagine what you would do when you discover another request a random other
> client sends that you would want to ignore just like you did for 'noop'.
> Viewed in this light, your patch is a very short sighted one that has a
> big negative impact on maintainability.

Fair enough. I was trying to change the minimal amount that I could and still fix the breakage. Your patch is much better. Not to mention terser ;-)

Show 8 quoted lines
> A true no-brainer that has no negative impact would have been something
> like the attached patch, that adds a method that does not do anything.
>
> Even then, between req_CATCHALL and req_EMPTY, I am not sure which one is
> expected by the clients, without consulting to the protocol documentation
> for cvs server/client communication.  In the attached patch, I am guessing
> from your patch that at least Tortoise does not expect any response to
> it.

I have consulted the CVS protocol documentation (found at http://www.wandisco.com/techpubs/cvs-protocol.pdf), which states the following about the "noop" command:

"Response expected: yes. This request is a null command in the sense that it doesn't do anything, but merely (as with any other requests expecting a response) sends back any responses pertaining to pending errors, pending Notified responses, etc."

So apparently a response *is* expected. I'm not really familiar enough with CVS or git-cvsserver to determine what that means it should do, but I suspect from perusing the code that req_EMPTY is the appropriate action.

Moreover, I've moved on from using git-cvsserver myself, having instead convinced my Windows-using compatriots to use msysgit instead. So if you feel that this change is unwarranted, feel free to just drop it.

Junio C Hamano· Jan 29, 2009, 23:46 UTC · re: Stefan Karpinski · lore

Re: [PATCH] git-cvsserver: handle CVS 'noop' command.

Stefan Karpinski <stefan.karpinski@gmail.com> writes:
Show 9 quoted lines
> So apparently a response *is* expected. I'm not really familiar enough
> with CVS or git-cvsserver to determine what that means it should do,
> but I suspect from perusing the code that req_EMPTY is the appropriate
> action.
>
> Moreover, I've moved on from using git-cvsserver myself, having
> instead convinced my Windows-using compatriots to use msysgit instead.
> So if you feel that this change is unwarranted, feel free to just drop
> it.

Because the issue currently has our attention, and we think we know that the code does not do the right thing currently, and that we are fairly sure that the right thing is to do req_EMPTY, I'd rather see a tested fix applied so that we can forget about it ;-)

It's good that you moved your people to native git environment, but if you have an environment where you can test the fix still lying around, I'd appreciate a quick test and resubmit.

Stefan Karpinski· Jan 30, 2009, 01:12 UTC · re: Junio C Hamano · lore

[PATCH] git-cvsserver: handle CVS 'noop' command.

The CVS protocol documentation, found at
  http://www.wandisco.com/techpubs/cvs-protocol.pdf
states the following about the 'noop' command:
  Response expected: yes. This request is a null command
  in the sense that it doesn't do anything, but merely
  (as with any other requests expecting a response) sends
  back any responses pertaining to pending errors, pending
  Notified responses, etc.

In accordance with this, the correct way to handle the 'noop' command, when issued by a client, is to call req_EMPTY.

The 'noop' command is called by some CVS clients, notably TortoiseCVS, thus making it desirable for git-cvsserver to respond to the command rather than choking on it as unknown.

Signed-off-by: Stefan Karpinski <stefan.karpinski@gmail.com>
---
On Thu, Jan 29, 2009 at 3:46 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 9 quoted lines
>
> Because the issue currently has our attention, and we think we know that
> the code does not do the right thing currently, and that we are fairly
> sure that the right thing is to do req_EMPTY, I'd rather see a tested fix
> applied so that we can forget about it ;-)
>
> It's good that you moved your people to native git environment, but if you
> have an environment where you can test the fix still lying around, I'd
> appreciate a quick test and resubmit.

I've done the best testing I could do under the circumstances. What that means is that the only windows machine I have access to test this on right now is running Vista, which is only partially (read poorly) supported by TortoiseCVS. So things seem to work well enough, but TortoiseCVS keeps crapping out for Vista-related reasons rather than git-cvsserver-related reasons. But I did manage to coax it into successfully checking out a complete working repository without the "noop" errors that it used to give.

 git-cvsserver.perl |    1 +
 1 files changed, 1 insertions(+), 0 deletions(-)
Show changes to git-cvsserver.perl +1 −0
diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index fef7faf..277ee4e 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -76,6 +76,7 @@ my $methods = {
     'history'         => \&req_CATCHALL,
     'watchers'        => \&req_EMPTY,
     'editors'         => \&req_EMPTY,
+    'noop'            => \&req_EMPTY,
     'annotate'        => \&req_annotate,
     'Global_option'   => \&req_Globaloption,
     #'annotate'        => \&req_CATCHALL,

-- 
1.6.0.3.3.g08dd8
Martin Langhoff· Jan 30, 2009, 01:32 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-cvsserver: handle CVS 'noop' command.

On Fri, Jan 30, 2009 at 11:45 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Not a no-brainer at all, sorry.
+1 on Junio's notes and patch.

Can someone with a real TortoiseCVS and a real cvs server sniff the connection and catch the noop? (Can TortoiseCVS write debug logs of the conversation with the server?)

Hysterical note: the original implementation of cvsserver was done reading the output of `cvs -t $opts $cmd`, and ocassionally sniffing the traffic on the wire or ssh connection.

Probably not a major issue for 'noop' though :-)
cheers,
m
-- 
 martin.langhoff@gmail.com
 martin@laptop.org -- School Server Architect
 - ask interesting questions
 - don't get distracted with shiny stuff  - working code first
 - http://wiki.laptop.org/go/User:Martinlanghoff

← back to recent threads