[PATCH] git-cvsexportcommit: handle file status reported out of order (was Re: [PATCH] Make git-cvsexportcommit "status" each file in turn)

[PATCH] git-cvsexportcommit: handle file status reported out of order (was Re: [PATCH] Make git-cvsexportcommit "status" each file in turn)

[Nick Woolley]

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

Hi,

I've encountered a problem with the Ubuntu Hardy version of 
git-cvsexportcommit (part of git-cvs 1:1.5.4.3-1ubuntu2). This seems to 
be the same problem describedin August last year in the thread on this 
list I referenced (Make git-cvsexportcommit "status" each file in turn).

The problem is that cvsexportcommit rejects a patch erroneously when CVS 
does not report file statuses in the expected order.  It confuses one 
file's status for another and says (at least in my case):

> Checking if patch will apply
> cvs server: nothing known about XXXXXXXXX
> cvs server: nothing known about XXXXXXXXX
> File XXXXXXXXX is already known in your CVS checkout   -- perhaps it\
 >  has been added by another user. Or this may indicate that it exists\
 >  on a different branch. If this is the case, use -f to force the\
 >  merge.
> Status was: Up-to-date
> File XXXXXXXXX not up to date but has status   'Unknown' in your\
 >  CVS checkout!
> Exiting: your CVS tree is not clean for this merge. at /usr/bin/git\
 >  -cvsexportcommit line 235.

I searched this list - including the release announcements - and the web 
and it did not seem that anyone had applied this patch or otherwise 
addressed it.

I then found the git release repository and it *does* seem to have been 
addressed in 1.6.0.  However, and correct me if I'm mistaken, but the 
implementation seems to assume that the committed files' basenames are 
unique?  This can't be guaranteed in general, can it?

Anyway, by then I'd already had a go at fixing it myself, and I had an 
alternative approach, partly based on the suggestions from Robin 
Rosenberg in the referenced thread. It doesn't assume basenames are 
unique, and therefore I think should be more robust.

So, attached is a patch against v1.5.4.  It works for me, but has only 
been tested with my particular circumstance, so there could be 
assumptions I made which aren't universally true.  I did try using the 
test suite in git 1.6.0 but it didn't work for reasons I don't want to 
spend too much more time investigating.  I think the patch as it is 
should be easy to follow and integrate with 1.6.0 for someone familiar 
with the codebase.

Some notes about the patch.

  - It parses the output of CVS status / update, getting the
    file status much as before

  - save_pipe_capture() is modified to capture STDERR as well as STDOUT

  - The STDERR message 'nothing known about <path>' is used
    preferentially to get a file's path.

  - Otherwise, uses the 'Repository revision' field, extracting the
    relevant part by removing the repository root constructed from
    meta-data in the CVS/Root and CVS/Repository files.

I hope this helps.  Comments welcome.

Thanks,

Nick


[-- Attachment #2: git-cvsexportcommit.diff --]
[-- Type: text/x-diff, Size: 3372 bytes --]

--- /usr/bin/git-cvsexportcommit.orig	2008-10-02 01:16:23.000000000 +0100
+++ /home/nick/bin/git-cvsexportcommit	2008-10-02 01:17:13.000000000 +0100
@@ -7,6 +7,18 @@
 use Data::Dumper;
 use File::Basename qw(basename dirname);
 
+# read in the first line of a file
+sub first_line {
+    my ($file) = @_;
+    my $line = '';
+   
+    return unless open my $fh, "<$file";
+    chomp ($line = <$fh>);
+    close $fh;
+    return $line;
+}
+
+
 our ($opt_h, $opt_P, $opt_p, $opt_v, $opt_c, $opt_f, $opt_a, $opt_m, $opt_d, $opt_u, $opt_w);
 
 getopts('uhPpvcfam:d:w:');
@@ -193,21 +205,63 @@
     push @canstatusfiles, $f;
 }
 
+
+# Get the root path the CVS working directory thinks its respository is at.
+# (We're in the working dir). Note we assume the repository agrees on this
+# throughout, which isn't necessarily true, but if not it's just bonkers.
+my $root_path = first_line("CVS/Root") || '';
+
+# likewise the module
+my $module_path = first_line("CVS/Repository") || '';
+        
+# try and strip off all but the path from the root - we assume the path has no colons
+$root_path =~ s/^.*://;
+$root_path =~ s{/*$}{/$module_path}; # concatenate the paths
+
+
+# lightly tested - seems ok with unknown files, and CVS repos using non-symbolic modules
+    
+# get the files' statuses
 my %cvsstat;
 if (@canstatusfiles) {
     if ($opt_u) {
       my @updated = xargs_safe_pipe_capture([@cvs, 'update'], @canstatusfiles);
       print @updated;
     }
+
     my @cvsoutput;
     @cvsoutput = xargs_safe_pipe_capture([@cvs, 'status'], @canstatusfiles);
-    my $matchcount = 0;
-    foreach my $l (@cvsoutput) {
-        chomp $l;
-        if ( $l =~ /^File:/ and  $l =~ /Status: (.*)$/ ) {
-            $cvsstat{$canstatusfiles[$matchcount]} = $1;
-            $matchcount++;
+ 
+    chomp @cvsoutput;
+    my ($status, $path, $repo_path);
+    my %expected_paths = map { $_ => 1 } @canstatusfiles;
+
+    # Use $_ implcitly here to simplify the regex expressions.
+    # Note that foreach saves and restores $_ for us
+    foreach (@cvsoutput) {
+        # Grab the information when we see it
+        $status = $1, next if /^File:.*Status:\s*(.*)/i;
+        $path = $1,   next if /^cvs server: nothing known about (.*)/i;
+
+        next unless m{Repository revision:[\s\d.]*(.*)}i;
+        $repo_path = $1;
+
+        # At this point we can try and reconstruct the file path
+        if (!$path && $repo_path !~ /No entry for/i) { 
+            $repo_path =~ s{Attic/?}{}i;
+            $repo_path =~ s{,v\s*$}{}i;
+            $repo_path =~ s{^$root_path/}{};
+            
+            $path = $repo_path;
         }
+
+        warn "Failed to get a path from the CVS status entry containing:\n$_"
+            unless $path;
+        warn "Found a path in the CVS output we didn't ask for: '$path'" 
+            unless exists $expected_paths{$path};
+                
+        $cvsstat{$path} = $status;
+        $path = $repo_path = $status = undef; 
     }
 }
 
@@ -331,7 +385,8 @@
 	@output = (<$child>);
 	close $child or die join(' ',@_).": $! $?";
     } else {
-	exec(@_) or die "$! $?"; # exec() can fail the executable can't be found
+        open STDERR, ">&STDOUT" or warn "child can't dup STDERR";;
+        exec(@_) or die "$! $?"; # exec() can fail the executable can't be found
     }
     return wantarray ? @output : join('',@output);
 }

Re: [PATCH] git-cvsexportcommit: handle file status reported out of order (was Re: [PATCH] Make git-cvsexportcommit "status" each file in turn)

[Johannes Schindelin]

Hi,

On Thu, 2 Oct 2008, Nick Woolley wrote:

> I've encountered a problem with the Ubuntu Hardy version of 
> git-cvsexportcommit (part of git-cvs 1:1.5.4.3-1ubuntu2). This seems to 
> be the same problem describedin August last year in the thread on this 
> list I referenced (Make git-cvsexportcommit "status" each file in turn).
> 
> The problem is that cvsexportcommit rejects a patch erroneously when CVS 
> does not report file statuses in the expected order.  It confuses one 
> file's status for another and says (at least in my case):
> 
> > Checking if patch will apply
> > cvs server: nothing known about XXXXXXXXX
> > cvs server: nothing known about XXXXXXXXX
> > File XXXXXXXXX is already known in your CVS checkout   -- perhaps it\
> >  has been added by another user. Or this may indicate that it exists\
> >  on a different branch. If this is the case, use -f to force the\
> >  merge.
> > Status was: Up-to-date
> > File XXXXXXXXX not up to date but has status   'Unknown' in your\
> >  CVS checkout!
> > Exiting: your CVS tree is not clean for this merge. at /usr/bin/git\
> >  -cvsexportcommit line 235.
> 
> I searched this list - including the release announcements - and the web 
> and it did not seem that anyone had applied this patch or otherwise 
> addressed it.
> 
> I then found the git release repository and it *does* seem to have been 
> addressed in 1.6.0.  However, and correct me if I'm mistaken, but the 
> implementation seems to assume that the committed files' basenames are 
> unique? This can't be guaranteed in general, can it?

Please research a bit better.  If the basenames are not unique, several 
cvs status calls are used.  See commit
fef3a7cc5593d3951a5f95c92986fb9982c2cc86.

> Anyway, by then I'd already had a go at fixing it myself, and I had an 
> alternative approach, partly based on the suggestions from Robin 
> Rosenberg in the referenced thread. It doesn't assume basenames are 
> unique, and therefore I think should be more robust.
> 
> So, attached is a patch against v1.5.4.  It works for me, but has only 
> been tested with my particular circumstance, so there could be 
> assumptions I made which aren't universally true.  I did try using the 
> test suite in git 1.6.0 but it didn't work for reasons I don't want to 
> spend too much more time investigating.  I think the patch as it is 
> should be easy to follow and integrate with 1.6.0 for someone familiar 
> with the codebase.
> 
> Some notes about the patch.
> 
>  - It parses the output of CVS status / update, getting the
>    file status much as before
> 
>  - save_pipe_capture() is modified to capture STDERR as well as STDOUT
> 
>  - The STDERR message 'nothing known about <path>' is used
>    preferentially to get a file's path.
> 
>  - Otherwise, uses the 'Repository revision' field, extracting the
>    relevant part by removing the repository root constructed from
>    meta-data in the CVS/Root and CVS/Repository files.
> 
> I hope this helps.  Comments welcome.

I can only assume that you have not really hung out on this list for very 
long; this is no way near the way patches are expected here.  For more 
help, refer to Documentation/SubmittingPatches, as has been mentioned in 
the notes from the maintainer quite a number of times.  Or imitate how 
other people submit their patches.

Also, given the fact that you actually verified that it was fixed in 
1.6.0, what exactly is your proposed course of action?  Revert the fix in 
1.6.0 and apply your patch?  Apply your patch to 1.5.6.5, cracking a 
release 1.5.6.6 with your patch?

Puzzled,
Dscho

Re: [PATCH] git-cvsexportcommit: handle file status reported out of order (was Re: [PATCH] Make git-cvsexportcommit "status" each file in turn)

[Nick Woolley]

Johannes Schindelin wrote:
> Please research a bit better.  If the basenames are not unique, several 
> cvs status calls are used.  See commit
> fef3a7cc5593d3951a5f95c92986fb9982c2cc86.

Yes, I see. I did spend some time searching for prior art on this issue, 
but I obviously wasn't looking in the right places.

> I can only assume that you have not really hung out on this list for very 
> long; this is no way near the way patches are expected here.

Correct.

> Also, given the fact that you actually verified that it was fixed in 
> 1.6.0, what exactly is your proposed course of action?  Revert the fix in 
> 1.6.0 and apply your patch?  Apply your patch to 1.5.6.5, cracking a 
> release 1.5.6.6 with your patch?

Neither. At the moment, I can only offer what I have: a patch for 
1.5.6.5, representing an idea that may contribute something small to 
1.6.0 (using information from CVS on STDERR). It should be simple to 
adapt if it's useful, if not please ignore it.

By posting, I hoped to learn if it was useful and where to look for the 
information I was missing, which you've been helpful enough to point out.

Thanks,

N

Re: [PATCH] git-cvsexportcommit: handle file status reported out of order (was Re: [PATCH] Make git-cvsexportcommit "status" each file in turn)

[Johannes Schindelin]

Hi,

On Thu, 2 Oct 2008, Nick Woolley wrote:

> Johannes Schindelin wrote:
> 
> > Also, given the fact that you actually verified that it was fixed in 
> > 1.6.0, what exactly is your proposed course of action?  Revert the fix 
> > in 1.6.0 and apply your patch?  Apply your patch to 1.5.6.5, cracking 
> > a release 1.5.6.6 with your patch?
> 
> Neither. At the moment, I can only offer what I have: a patch for 
> 1.5.6.5, representing an idea that may contribute something small to 
> 1.6.0 (using information from CVS on STDERR). It should be simple to 
> adapt if it's useful, if not please ignore it.

Since you did not use the preferred form of a patch, you also do not have 
a commit message describing the idea of your patch.  It is a bit hard to 
find out the intention from reading the source, and I am not motivated 
enough to find out, given that it is fixed with the commit I referred you 
to.

To save you time: the idea of the commit I referred you to is not exactly 
to use the basename, but the trimmed names.  Now only sets of unique names 
(where it is also checked that a name could not be mistaken for another, 
deleted file) are passed to cvs status.

BTW next time you search for some fix in Git's source code, you might want 
to use either "git log -- <file>" or "git bisect" to find the commit in 
question.

Hth,
Dscho