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

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

[Stefan Karpinski]

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:
>
> 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
>

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

[Junio C Hamano]

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.

> 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

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

[Stefan Karpinski]

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(-)

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

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

[Junio C Hamano]

Stefan Karpinski <stefan.karpinski@gmail.com> writes:

> 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.

>  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

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

[Stefan Karpinski]

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.

> 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.

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

[Andy Parkins]

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

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

[Stefan Karpinski]

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

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

[Junio C Hamano]

Stefan Karpinski <stefan.karpinski@gmail.com> writes:

> 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(-)

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,

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

[Stefan Karpinski]

On Thu, Jan 29, 2009 at 2:45 PM, Junio C Hamano <gitster@pobox.com> wrote:
>
> 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 ;-)

> 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.

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

[Junio C Hamano]

Stefan Karpinski <stefan.karpinski@gmail.com> writes:

> 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.

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

[Stefan Karpinski]

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:
>
> 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(-)

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

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

[Martin Langhoff]

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