threads / patch / 11189

patchCalculate $commitsha1 in update() only when needed

Subject: [PATCH] Calculate $commitsha1 in update() only when needed

## tl;dr

3 messages between Dec 8, 2007 and Dec 8, 2007. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Pavel Roskin· Dec 8, 2007, 05:07 UTC · lore

This suppresses unhelpful error messages from git rev-parse during checkout if the module doesn't exist.

Signed-off-by: Pavel Roskin <proski@gnu.org>
---

 git-cvsserver.perl |   12 +++++++-----
 1 files changed, 7 insertions(+), 5 deletions(-)

Show changes to git-cvsserver.perl +7 −5

diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index ecded3b..409b301 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -2427,9 +2427,6 @@ sub update
     # first lets get the commit list
     $ENV{GIT_DIR} = $self->{git_path};
 
-    my $commitsha1 = `git rev-parse $self->{module}`;
-    chomp $commitsha1;
-
     my $commitinfo = `git cat-file commit $self->{module} 2>&1`;
     unless ( $commitinfo =~ /tree\s+[a-zA-Z0-9]{40}/ )
     {
@@ -2440,8 +2437,13 @@ sub update
     my $git_log;
     my $lastcommit = $self->_get_prop("last_commit");
 
-    if (defined $lastcommit && $lastcommit eq $commitsha1) { # up-to-date
-         return 1;
+    if (defined $lastcommit) {
+        my $commitsha1 = `git rev-parse $self->{module}`;
+        chomp $commitsha1;
+
+        if ($lastcommit eq $commitsha1) { # up-to-date
+            return 1;
+        }
     }
 
     # Start exclusive lock here...

Junio C Hamano· Dec 8, 2007, 08:17 UTC · re: Pavel Roskin · lore

Re: [PATCH] Calculate $commitsha1 in update() only when needed

Pavel Roskin <proski@gnu.org> writes:

Show 14 quoted lines

> diff --git a/git-cvsserver.perl b/git-cvsserver.perl
> index ecded3b..409b301 100755
> --- a/git-cvsserver.perl
> +++ b/git-cvsserver.perl
> @@ -2427,9 +2427,6 @@ sub update
>      # first lets get the commit list
>      $ENV{GIT_DIR} = $self->{git_path};
>  
> -    my $commitsha1 = `git rev-parse $self->{module}`;
> -    chomp $commitsha1;
> -
>      my $commitinfo = `git cat-file commit $self->{module} 2>&1`;
>      unless ( $commitinfo =~ /tree\s+[a-zA-Z0-9]{40}/ )
>      {

Hmm. The first rev-parse could be squelched with 2>/dev/null and then you can check if it does not match [a-f0-9]{40} and die early before running "cat-file commit", can't you?

Also the regexp to check "tree" object name above seems quite wrong ;-) If the purpose of this check is to make sure if the ref points at a commit object, perhaps...

	my $commitsha1 = `git rev-parse --verify $self->{module}^0 2>&1`;
	chomp($commitsha1);
        if ($commitsha1 !~ /^[0-9a-f]{40}$/) {
        	die "no such module $self->{module}";
	}

Then the other hunk below would become unnecessary, I think.

Show 16 quoted lines

> @@ -2440,8 +2437,13 @@ sub update
>      my $git_log;
>      my $lastcommit = $self->_get_prop("last_commit");
>  
> -    if (defined $lastcommit && $lastcommit eq $commitsha1) { # up-to-date
> -         return 1;
> +    if (defined $lastcommit) {
> +        my $commitsha1 = `git rev-parse $self->{module}`;
> +        chomp $commitsha1;
> +
> +        if ($lastcommit eq $commitsha1) { # up-to-date
> +            return 1;
> +        }
>      }
>  
>      # Start exclusive lock here...

Pavel Roskin· Dec 8, 2007, 08:48 UTC · re: Junio C Hamano · lore

Re: [PATCH] Calculate $commitsha1 in update() only when needed

Quoting Junio C Hamano <gitster@pobox.com>:

Show 20 quoted lines

> Pavel Roskin <proski@gnu.org> writes:
>
>> diff --git a/git-cvsserver.perl b/git-cvsserver.perl
>> index ecded3b..409b301 100755
>> --- a/git-cvsserver.perl
>> +++ b/git-cvsserver.perl
>> @@ -2427,9 +2427,6 @@ sub update
>>      # first lets get the commit list
>>      $ENV{GIT_DIR} = $self->{git_path};
>>
>> -    my $commitsha1 = `git rev-parse $self->{module}`;
>> -    chomp $commitsha1;
>> -
>>      my $commitinfo = `git cat-file commit $self->{module} 2>&1`;
>>      unless ( $commitinfo =~ /tree\s+[a-zA-Z0-9]{40}/ )
>>      {
>
> Hmm.  The first rev-parse could be squelched with 2>/dev/null and then
> you can check if it does not match [a-f0-9]{40} and die early before
> running "cat-file commit", can't you?

Yes, my impression is that the code in question can be improved a lot.

This is specifically the error message I'd like to see fixed in some way, as it's confusing to beginners trying to check out the module for the first time.

$ CVS_SERVER=/home/proski/bin/git-cvsserver cvs -d \
  :fork:/home/proski/src/qgit/.git co foo
fatal: ambiguous argument 'foo': unknown revision or path not in the  
working tree.
Use '--' to separate paths from revisions
Invalid module 'foo' at /home/proski/bin/git-cvsserver line 2437,  
<STDIN> line 15.
cvs [checkout aborted]: end of file from server (consult above  
messages if any)

It's possible that the message about "--" makes sense and it should actually be added in some spaces.

-- 
Regards,
Pavel Roskin