[PATCH] Calculate lines changed for cvs log command

[PATCH] Calculate lines changed for cvs log command

[Fredrik Noring]

[-- Attachment #1: Type: text/plain, Size: 800 bytes --]

Hi,

Attached patch attempts to report the correct lines changed when using  
the cvs log command with cvsserver. Is there any performance impact  
using "git-log -1 --shortstat <object>" instead of "git-cat-file  
commit <object>"?

(Also, it would be nice if git-log --pretty=format:XXX could display  
diff information such as shortstat, to make parsing more robust.)

Previously, cvs log reported "+2 -3" for all commits. This patch  
reports real values except when the commit is fetched from the SQLite  
DB, which remains "+2 -3" at all times.

(My mailer destroys inline patches, so I'm attaching it. Sorry about  
that.)

Signed-off-by: Fredrik Noring <noring@nocrew.org>
---
  git-cvsserver.perl |   32 ++++++++++++++++++++++----------
  1 files changed, 22 insertions(+), 10 deletions(-)


[-- Attachment #2: 0001-Calculate-lines-changed-for-cvs-log-command.patch --]
[-- Type: application/octet-stream, Size: 3274 bytes --]

From 6ff024e5238e52f4b61226187ffa2d21b99fe081 Mon Sep 17 00:00:00 2001
From: Fredrik Noring <noring@nocrew.org>
Date: Sun, 13 Apr 2008 16:54:40 +0200
Subject: [PATCH] Calculate lines changed for cvs log command

Previously, "+2 -3" was reported for all commits. This patch reports
lines changed except when the commit is fetched from the SQLite DB
(which remains "+2 -3" at all times).

Signed-off-by: Fredrik Noring <noring@nocrew.org>
---
 git-cvsserver.perl |   32 ++++++++++++++++++++++----------
 1 files changed, 22 insertions(+), 10 deletions(-)

diff --git a/git-cvsserver.perl b/git-cvsserver.perl
index 29dbfc9..5d31a95 100755
--- a/git-cvsserver.perl
+++ b/git-cvsserver.perl
@@ -1729,8 +1729,9 @@ sub req_log
             # reformat the date for log output
             $revision->{modified} = sprintf('%04d/%02d/%02d %s', $3, $DATE_LIST->{$2}, $1, $4 ) if ( $revision->{modified} =~ /(\d+)\s+(\w+)\s+(\d+)\s+(\S+)/ and defined($DATE_LIST->{$2}) );
             $revision->{author} = cvs_author($revision->{author});
-            print "M date: $revision->{modified};  author: $revision->{author};  state: " . ( $revision->{filehash} eq "deleted" ? "dead" : "Exp" ) . ";  lines: +2 -3\n";
-            my $commitmessage = $updater->commitmessage($revision->{commithash});
+            ( my $commitmessage, my $lines_changed ) =
+		$updater->commitinfo($revision->{commithash});
+            print "M date: $revision->{modified};  author: $revision->{author};  state: " . ( $revision->{filehash} eq "deleted" ? "dead" : "Exp" ) . ";  lines: " . $lines_changed . "\n";
             $commitmessage =~ s/^/M /mg;
             print $commitmessage . "\n";
         }
@@ -3023,12 +3024,13 @@ sub getmeta
     return $db_query->fetchrow_hashref;
 }
 
-=head2 commitmessage
+=head2 commitinfo
 
-this function takes a commithash and returns the commit message for that commit
+this function takes a commithash and returns the commit message and
+lines changed for that commit
 
 =cut
-sub commitmessage
+sub commitinfo
 {
     my $self = shift;
     my $commithash = shift;
@@ -3045,14 +3047,24 @@ sub commitmessage
     if ( defined ( $message ) )
     {
         $message .= " " if ( $message =~ /\n$/ );
-        return $message;
+        return ( $message, "+2 -3" );   # FIXME: What's the real lines changed?
     }
 
-    my @lines = safe_pipe_capture("git-cat-file", "commit", $commithash);
-    shift @lines while ( $lines[0] =~ /\S/ );
-    $message = join("",@lines);
+    my @lines = safe_pipe_capture("git-log", "-1", "--pretty=format:%s%n%b",
+				  "--shortstat", $commithash);
+    my $files_changed = 0;
+    my $insertions = 0;
+    my $deletions = 0;
+    ( $files_changed, $insertions, $deletions ) = ( $lines[-1] =~ / (\d+) files changed, (\d+) insertions.*, (\d+) deletions/ );
+    if ( $files_changed )
+    {
+	pop(@lines);   # Remove shortstat
+	pop(@lines);   # Remove extra newline prior to shortstat
+    }
+    map(s/\n$//s, @lines);
+    $message = join("\n", @lines);
     $message .= " " if ( $message =~ /\n$/ );
-    return $message;
+    return ( $message, sprintf("+%d -%d", $insertions, $deletions) );
 }
 
 =head2 gethistory
-- 
1.5.5.40.g4cdda.dirty


[-- Attachment #3: Type: text/plain, Size: 1 bytes --]

Re: [PATCH] Calculate lines changed for cvs log command

[Jakub Narebski]

Fredrik Noring <noring@nocrew.org> writes:

> (My mailer destroys inline patches, so I'm attaching it. Sorry about
> that.)

Could you please attach it as 1.) attachement=inline if possible, to
have them displayed along the email; 2.) use text/plain mimetype, not
application/octet-stream (you might need to change patch extension
from *.patch to *.txt; you can change default extension of files
generated by git-format-patch via format.suffix configuration
variable), and what is tied with it 3.) use Transfer-Encoding: 8bit
(or something like that), not base64, and preferably not
quoted-printable?

Or change your mailer (or configure it), or use git-send-email.
-- 
Jakub Narebski
Poland
ShadeHawk on #git

Re: [PATCH] Calculate lines changed for cvs log command

[Fredrik Noring]

Hi Jakub,

OK, I'll look deeper into the mailer issues. Any comments on the  
actual code in the patch, and the question regarding git-log vs git- 
cat-file?

Speaking of which -- is there any particular reason for not running  
git-log once, instead of forking it (or git-cat-file) for every commit  
in the log? There seems to be a special case for merges, but it'd  
appear to be more efficient to query the SQLite DB for the commits  
provided by git-log rather than the other way around, no?

Any thoughts?

(git-send-email appears to fail for me without providing an error.)

Many thanks,
Fredrik

13 apr 2008 kl. 18.59 skrev Jakub Narebski:
> Fredrik Noring <noring@nocrew.org> writes:
>
>> (My mailer destroys inline patches, so I'm attaching it. Sorry about
>> that.)
>
> Could you please attach it as 1.) attachement=inline if possible, to
> have them displayed along the email; 2.) use text/plain mimetype, not
> application/octet-stream (you might need to change patch extension
> from *.patch to *.txt; you can change default extension of files
> generated by git-format-patch via format.suffix configuration
> variable), and what is tied with it 3.) use Transfer-Encoding: 8bit
> (or something like that), not base64, and preferably not
> quoted-printable?
>
> Or change your mailer (or configure it), or use git-send-email.
> -- 
> Jakub Narebski
> Poland
> ShadeHawk on #git
>

Re: [PATCH] Calculate lines changed for cvs log command

[Jakub Narebski]

Could you please do not toppost?  It is against natural reading order.

Dnia niedziela 13. kwietnia 2008 20:44, Fredrik Noring napisał:
> 13 apr 2008 kl. 18.59 skrev Jakub Narebski:
>> Fredrik Noring <noring@nocrew.org> writes:
>>
>>> (My mailer destroys inline patches, so I'm attaching it. Sorry about
>>> that.)
>>
>> Could you please attach it as 1.) attachement=inline if possible, to
>> have them displayed along the email; 2.) use text/plain mimetype, not
>> application/octet-stream (you might need to change patch extension
>> from *.patch to *.txt; you can change default extension of files
>> generated by git-format-patch via format.suffix configuration
>> variable), and what is tied with it 3.) use Transfer-Encoding: 8bit
>> (or something like that), not base64, and preferably not
>> quoted-printable?
>>
>> Or change your mailer (or configure it), or use git-send-email.
>> 
> OK, I'll look deeper into the mailer issues. Any comments on the  
> actual code in the patch, and the question regarding git-log vs git- 
> cat-file?

First, having patch as attachement, moreover as _binary_ and _encoded_ 
attachement makes it hard to comment on it.  And if you make it hard to 
comment on patch, people woundn't do it.

Second, your approach with "--pretty=format:%s%n%b" might be a good
idea, but you don't get all the information as you get from 
git-cat-file: no tree info, no true (recorded) parents info, no 
committer info, no author info.  I don't know if git-cvsserver makes 
use of that info or not; from the patch it looks as if it discards
all but commit message (message body). 

BTW. I'm not Perl expert, but if you want to discard two last
elements in arrays, wouldn't using splice (or just decreasing
$#lines by 2) be simpler solution?

> Speaking of which -- is there any particular reason for not running  
> git-log once, instead of forking it (or git-cat-file) for every commit  
> in the log? There seems to be a special case for merges, but it'd  
> appear to be more efficient to query the SQLite DB for the commits  
> provided by git-log rather than the other way around, no?

About git-log vs git-cat-file: take a look how gitweb does it, with 
parse_commits and parse_commit_text, and commit 756bbf548dbef5b738c
by Robert Fitzsimons introducing it.  Note that IIRC this commit
predates --pretty=format:<fmt> option to git-log / git-rev-list.

> Any thoughts?
> 
> (git-send-email appears to fail for me without providing an error.)

You have either configure sendmail, or set mail.smtp* options (or 
something like that) correctly.

BTW. cannot you turn off "format=flowed" in Apple Mail?
-- 
Jakub Narebski
Poland

Re: [PATCH] Calculate lines changed for cvs log command

[Fredrik Noring]

Hi Jakub,

13 apr 2008 kl. 22.43 skrev Jakub Narebski:
> First, having patch as attachement, moreover as _binary_ and _encoded_
> attachement makes it hard to comment on it.  And if you make it hard  
> to
> comment on patch, people woundn't do it.

Again, my sincere apologies. I have a feeling my current mailer never  
will meet the patch submission requirements for this mailing list.  
However, I will replace it at the next opportunity in order to comply.

The goal with these patches is to make cvsserver commands such as "cvs  
log" identical to the real thing, so that cvs users would feel at home  
after a cvs import to git. I'm getting closer with these tweaks/fixes  
to the cvsserver. So far mainly handling of whitespace and calculating  
proper "shortstats". Decent performance would be nice as well. I'm  
playing around with this but I'm in no hurry, so feel free to ignore. :)

> Second, your approach with "--pretty=format:%s%n%b" might be a good
> idea, but you don't get all the information as you get from
> git-cat-file: no tree info, no true (recorded) parents info, no
> committer info, no author info.  I don't know if git-cvsserver makes
> use of that info or not; from the patch it looks as if it discards
> all but commit message (message body).

Yeah -- it's all discarded. A small issue is that "--pretty=format:%s%n 
%b" only outputs the same commit message as git-cat-file in ~95 % of  
my test cases. I've seen differences regarding newlines after the  
subject line (git-cat-file seems to output this correctly, but not %s%n 
%b). Something to investigate further, perhaps.

> BTW. I'm not Perl expert, but if you want to discard two last
> elements in arrays, wouldn't using splice (or just decreasing
> $#lines by 2) be simpler solution?

I went for clarity and felt pop() made the job. ;)

> About git-log vs git-cat-file: take a look how gitweb does it, with
> parse_commits and parse_commit_text, and commit 756bbf548dbef5b738c
> by Robert Fitzsimons introducing it.  Note that IIRC this commit
> predates --pretty=format:<fmt> option to git-log / git-rev-list.

Sure, thanks!

> BTW. cannot you turn off "format=flowed" in Apple Mail?

Well, it appears easier to replace it altogether in order to follow  
the patch submission rules. I like Gnome Evolution and it'll hopefully  
work nicely in this case.

> Could you please do not toppost?  It is against natural reading order.

>

Heh. Very funny you wrote that as a toppost yourself. ;)

Thanks,
Fredrik