threads / patch / 19565

patchgit-cvsexportcommit can't commit files which have been removed from CVS

Subject: [PATCH] git-cvsexportcommit can't commit files which have been removed from CVS

## tl;dr

9 messages between May 28, 2009 and Jul 6, 2009. Diffs are folded; open one to read it.

replies: 8people: 4as markdown or json

Nick Woolley· May 28, 2009, 23:23 UTC · lore

If a file X is removed from CVS, it goes into the Attic directory, and CVS reports it as 'no file X' but with status 'Up-to-date'. This is misinterpreted an existing file when git-cvsexportcommit tries to commit a file with the same name as one of these. This patch attempts to correctly identify these files, so that new files with the same name can be committed.

Added a test to t9200-git-cvsexportcommit.sh, which tests that we can re-commit a removed filename which remains in CVS's attic. This adds a file 'attic_gremlin' in CVS, then "removes" it, then tries to commit a file with the same name from git.

Signed-off-by: Nick Woolley <git.wu-lee@noodlefactory.co.uk>
---
 git-cvsexportcommit.perl       |   49 ++++++++++++++++++++++++++++-----------
 t/t9200-git-cvsexportcommit.sh |   18 ++++++++++++++
 2 files changed, 53 insertions(+), 14 deletions(-)
Show changes to 2 files +53 −14

git-cvsexportcommit.perl, t/t9200-git-cvsexportcommit.sh

diff --git a/git-cvsexportcommit.perl b/git-cvsexportcommit.perl
index 6d9f0ef..e5e8ca9 100755
--- a/git-cvsexportcommit.perl
+++ b/git-cvsexportcommit.perl
@@ -225,7 +225,14 @@ if (@canstatusfiles) {
       foreach my $name (keys %todo) {
 	my $basename = basename($name);

-	$basename = "no file " . $basename if (exists($added{$basename}));
+	# CVS reports files which are "status"ed which don't exist
+	# in the current revision as "no file $basename", so we
+	# should anticipate that.  Totally unknown files will have
+	# a status "Unknown". However, if they exist in the attic
+	# their status will be "Up-to-date" (this means they were
+	# added once but have been removed).
+	$basename = "no file $basename" if $added{$basename};
+
 	$basename =~ s/^\s+//;
 	$basename =~ s/\s+$//;

@@ -233,31 +240,45 @@ if (@canstatusfiles) {
 	  $fullname{$basename} = $name;
 	  push (@canstatusfiles2, $name);
 	  delete($todo{$name});
-        }
+	}
       }
       my @cvsoutput;
       @cvsoutput = xargs_safe_pipe_capture([@cvs, 'status'], @canstatusfiles2);
       foreach my $l (@cvsoutput) {
-        chomp $l;
-        if ($l =~ /^File:\s+(.*\S)\s+Status: (.*)$/) {
-	  if (!exists($fullname{$1})) {
-	    print STDERR "Huh? Status reported for unexpected file '$1'\n";
-	  } else {
-	    $cvsstat{$fullname{$1}} = $2;
-	  }
-	}
+	chomp $l;
+	next unless
+	    my ($file, $status) = $l =~ /^File:\s+(.*\S)\s+Status: (.*)$/;
+
+	my $fullname = $fullname{$file};
+	print STDERR "Huh? Status '$status' reported for unexpected file '$file'\n"
+	    unless defined $fullname;
+
+	# This response means the file does not exist except in
+	# CVS's attic, so set the status accordingly
+	$status = "In-attic"
+	    if $file =~ /^no file /
+		&& $status eq 'Up-to-date';
+
+	$cvsstat{$fullname{$file}} = $status;
       }
     }
 }

-# ... validate new files,
+# ... Validate new files have the correct status
 foreach my $f (@afiles) {
-    if (defined ($cvsstat{$f}) and $cvsstat{$f} ne "Unknown") {
-	$dirty = 1;
+    next unless defined(my $stat = $cvsstat{$f});
+
+    # This means the file has never been seen before
+    next if $stat eq 'Unknown';
+
+    # This means the file has been seen before but was removed
+    next if $stat eq 'In-attic';
+
+    $dirty = 1;
 	warn "File $f 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.\n";
 	warn "Status was: $cvsstat{$f}\n";
-    }
 }
+
 # ... validate known files.
 foreach my $f (@files) {
     next if grep { $_ eq $f } @afiles;
diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh
index 56b7c06..ef1f8d2 100755
--- a/t/t9200-git-cvsexportcommit.sh
+++ b/t/t9200-git-cvsexportcommit.sh
@@ -317,4 +317,22 @@ test_expect_success 'use the same checkout for Git and CVS' '

 '

+test_expect_success 're-commit a removed filename which remains in CVS attic' '
+
+    (cd "$CVSWORK" &&
+     echo >attic_gremlin &&
+     cvs -Q add attic_gremlin &&
+     cvs -Q ci -m "added attic_gremlin" &&
+     rm attic_gremlin &&
+     cvs -Q rm attic_gremlin &&
+     cvs -Q ci -m "removed attic_gremlin") &&
+
+    echo > attic_gremlin &&
+    git add attic_gremlin &&
+    git commit -m "Added attic_gremlin" &&
+	git cvsexportcommit -w "$CVSWORK" -c HEAD &&
+    (cd "$CVSWORK"; cvs -Q update -d) &&
+    test -f "$CVSWORK/attic_gremlin"
+'
+
 test_done
Mike Ralphson· Jun 10, 2009, 08:06 UTC · re: Nick Woolley · lore

Re: [PATCH] git-cvsexportcommit can't commit files which have been removed from CVS

2009/5/29 Nick Woolley <nickwoolley@yahoo.co.uk>
> Added a test to t9200-git-cvsexportcommit.sh, which tests that we can
> re-commit a removed filename which remains in CVS's attic. This adds a
> file 'attic_gremlin' in CVS, then "removes" it, then tries to commit a
> file with the same name from git.

Hi Nick, I'm seeing intermittent failures since your new test was added to 'next' on AIX 5.3

cvs commit: Up-to-date check failed for ` space'
cvs [commit aborted]: correct above errors first!
* FAIL 15: re-commit a removed filename which remains in CVS attic
* failed 1 among 15 test(s)
Is there a possibility this test has a race condition?
Let me know if there's anything I can do to help debug it.
It could be a bug in the ancient CVS I have here (1.11.1p1) though.
cvs status: Examining .
===================================================================
File:  space            Status: Needs Patch

As it's a file from the previous tests being fingered, I've cc'd a couple of likely lads.

Mike
Nick Woolley· Jun 11, 2009, 14:10 UTC · re: Mike Ralphson · lore

Re: [PATCH] git-cvsexportcommit can't commit files which have been removed from CVS

Mike Ralphson wrote:
Show 9 quoted lines
> Hi Nick, I'm seeing intermittent failures since your new test was
> added to 'next' on AIX 5.3
> 
> cvs commit: Up-to-date check failed for ` space'
> cvs [commit aborted]: correct above errors first!
> * FAIL 15: re-commit a removed filename which remains in CVS attic
> * failed 1 among 15 test(s)
> 
> Is there a possibility this test has a race condition?
Hm, I have thought not, but what sort of a race condition did you have in mind?
> Let me know if there's anything I can do to help debug it.
> 
> It could be a bug in the ancient CVS I have here (1.11.1p1) though.
I wouldn't be surprised.

What my fix does is to spot files marked "Up-to-date" but which have "no file " prepended to the filename in cvs status's output, which means the file used to exist but was deleted from the repository, not that it is "Up-to-date" in the sense you'd expect.

If old versions of CVS report these files differently they may not be spotted.

I don't know why the " space" file seems to be causing a problem - it shouldn't interfere with the test I added (and indeed doesn't for me).

Perhaps you could apply the following patch to t/t9200-git-cvsexportcommit.sh, run it, and send the contents of t/debug.out? What I get is appended after the patch.

You might also try commenting out the following part of my test, so that it should trivially work, and see if there's still an error:

# rm attic_gremlin && # cvs -Q rm attic_gremlin && # cvs -Q ci -m "removed attic_gremlin"

Cheers,
N
Show changes to t/t9200-git-cvsexportcommit.sh +8 −1
diff --git a/t/t9200-git-cvsexportcommit.sh b/t/t9200-git-cvsexportcommit.sh
index ef1f8d2..4f19a47 100755
--- a/t/t9200-git-cvsexportcommit.sh
+++ b/t/t9200-git-cvsexportcommit.sh
@@ -320,12 +320,19 @@ test_expect_success 'use the same checkout for Git and CVS' '
 test_expect_success 're-commit a removed filename which remains in CVS attic' '

     (cd "$CVSWORK" &&
+     dlog=../../debug.out &&
+     echo "# before adding file"  >$dlog &&
+     cvs status attic_gremlin " space" >>$dlog 2>&1 &&
+
      echo >attic_gremlin &&
      cvs -Q add attic_gremlin &&
      cvs -Q ci -m "added attic_gremlin" &&
      rm attic_gremlin &&
      cvs -Q rm attic_gremlin &&
-     cvs -Q ci -m "removed attic_gremlin") &&
+     cvs -Q ci -m "removed attic_gremlin" &&
+
+     echo "# after adding file"  >>$dlog &&
+     cvs status attic_gremlin " space" >>$dlog 2>&1) &&

     echo > attic_gremlin &&
     git add attic_gremlin &&

----
# before adding file
cvs status: nothing known about `attic_gremlin'
===================================================================
File: no file attic_gremlin		Status: Unknown

   Working revision:	No entry for attic_gremlin
   Repository revision:	No revision control file

===================================================================
File:  space           	Status: Up-to-date

   Working revision:	1.1	2009-06-11 13:56:15 +0000
   Repository revision:	1.1	/home/nick/gitworking/git/t/trash
directory.t9200-git-cvsexportcommit/cvsroot/ space,v
   Commit Identifier:	oDOEzyj3FzAazrRt
   Sticky Tag:		(none)
   Sticky Date:		(none)
   Sticky Options:	(none)

# after adding file
===================================================================
File: no file attic_gremlin		Status: Up-to-date

   Working revision:	No entry for attic_gremlin
   Repository revision:	1.2	/home/nick/gitworking/git/t/trash
directory.t9200-git-cvsexportcommit/cvsroot/Attic/attic_gremlin,v
   Commit Identifier:	X1mPOnzvk5rczrRt

===================================================================
File:  space           	Status: Up-to-date

   Working revision:	1.1	2009-06-11 13:56:15 +0000
   Repository revision:	1.1	/home/nick/gitworking/git/t/trash
directory.t9200-git-cvsexportcommit/cvsroot/ space,v
   Commit Identifier:	oDOEzyj3FzAazrRt
   Sticky Tag:		(none)
   Sticky Date:		(none)
   Sticky Options:	(none)
Junio C Hamano· Jun 11, 2009, 15:49 UTC · re: Nick Woolley · lore

Re: [PATCH] git-cvsexportcommit can't commit files which have been removed from CVS

Nick Woolley <nickwoolley@yahoo.co.uk> writes:
Show 18 quoted lines
> Mike Ralphson wrote:
>> Hi Nick, I'm seeing intermittent failures since your new test was
>> added to 'next' on AIX 5.3
>> 
>> cvs commit: Up-to-date check failed for ` space'
>> cvs [commit aborted]: correct above errors first!
>> * FAIL 15: re-commit a removed filename which remains in CVS attic
>> * failed 1 among 15 test(s)
>> 
>> Is there a possibility this test has a race condition?
>
> Hm, I have thought not, but what sort of a race condition did you have in mind?
>
>> Let me know if there's anything I can do to help debug it.
>> 
>> It could be a bug in the ancient CVS I have here (1.11.1p1) though.
>
> I wouldn't be surprised.

I just saw this on a k.org machine that runs Fedora 9 (Sulphur); 1.11.22 is the version of CVS that comes with it.

But it does seem to be repeatable; I wouldn't rule out a race condition.
Junio C Hamano· Jun 11, 2009, 17:04 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-cvsexportcommit can't commit files which have been removed from CVS

Junio C Hamano <gitster@pobox.com> writes:
Show 7 quoted lines
> Nick Woolley <nickwoolley@yahoo.co.uk> writes:
>> Mike Ralphson wrote:
>>> cvs commit: Up-to-date check failed for ` space'
>>> cvs [commit aborted]: correct above errors first!
>>> * FAIL 15: re-commit a removed filename which remains in CVS attic
>>> * failed 1 among 15 test(s)
> But it does seem to be repeatable; I wouldn't rule out a race condition.

Ehh, sorry, it "does not" seem to be repeatable. Sometimes and only sometimes it fails...

Robin Rosenberg· Jun 12, 2009, 06:59 UTC · re: Junio C Hamano · lore

Re: [PATCH] git-cvsexportcommit can't commit files which have been removed from CVS

torsdag 11 juni 2009 19:04:54 skrev Junio C Hamano <gitster@pobox.com>:
Show 12 quoted lines
> Junio C Hamano <gitster@pobox.com> writes:
> 
> > Nick Woolley <nickwoolley@yahoo.co.uk> writes:
> >> Mike Ralphson wrote:
> >>> cvs commit: Up-to-date check failed for ` space'
> >>> cvs [commit aborted]: correct above errors first!
> >>> * FAIL 15: re-commit a removed filename which remains in CVS attic
> >>> * failed 1 among 15 test(s)
> > But it does seem to be repeatable; I wouldn't rule out a race condition.
> 
> Ehh, sorry, it "does not" seem to be repeatable.  Sometimes and only
> sometimes it fails...

CVS har a timestamp in CVS/Entries that has whole second resolution. In addition it's built-in method for trying to work around the problem by sleeping until the next second wraps around is flaky. cvsexportcommit already has one sleep for this. Maybe it's not enough.

-- robin
Mike Ralphson· Jul 2, 2009, 13:50 UTC · re: Nick Woolley · lore

Re: [PATCH] git-cvsexportcommit can't commit files which have been removed from CVS

2009/6/11 Nick Woolley <nickwoolley@yahoo.co.uk>
Show 19 quoted lines
>
> Mike Ralphson wrote:
> > Hi Nick, I'm seeing intermittent failures since your new test was
> > added to 'next' on AIX 5.3
> >
> > cvs commit: Up-to-date check failed for ` space'
> > cvs [commit aborted]: correct above errors first!
> > * FAIL 15: re-commit a removed filename which remains in CVS attic
> > * failed 1 among 15 test(s)
> >
> > Let me know if there's anything I can do to help debug it.
> >
>
> I don't know why the  " space" file seems to be causing a problem - it shouldn't
> interfere with the test I added (and indeed doesn't for me).
>
> Perhaps you could apply the following patch to t/t9200-git-cvsexportcommit.sh,
> run it, and send the contents of t/debug.out?  What I get is appended after the
> patch.
On a failed run, the error is on the commit adding attic_gremlin:
    echo > attic_gremlin &&
    git add attic_gremlin &&
    git commit -m "Added attic_gremlin" &&
        git cvsexportcommit -w "$CVSWORK" -c HEAD &&
    (cd "$CVSWORK"; cvs -Q update -d) &&
    test -f "$CVSWORK/attic_gremlin"
cvs commit: Up-to-date check failed for ` space'
cvs [commit aborted]: correct above errors first!
* FAIL 15: re-commit a removed filename which remains in CVS attic
debug.out contains the following:
# before adding file
cvs status: nothing known about attic_gremlin
===================================================================
File: no file attic_gremlin             Status: Unknown
   Working revision:    No entry for attic_gremlin
   Repository revision: No revision control file
===================================================================
File:  space            Status: Needs Patch
   Working revision:    1.1     Thu Jul  2 12:50:17 2009
   Repository revision: 1.2     /usr/local/src/gitbuild/t/trash
directory.t9200-git-cvsexportcommit/cvsroot/ space,v
   Sticky Tag:          (none)
   Sticky Date:         (none)
   Sticky Options:      (none)
===
the " space" file in cvswork is:

-rw-rw---- 1 mike staff 6 2009-07-02 13:50:17.000000000 +0100 space

CVS/Entries for the file has / space/1.1/Thu Jul 2 12:50:17 2009//

Show 6 quoted lines
> You might also try commenting out the following part of my test, so that it
> should trivially work, and see if there's still an error:
>
> #      rm attic_gremlin &&
> #      cvs -Q rm attic_gremlin &&
> #      cvs -Q ci -m "removed attic_gremlin"
It all goes a bit fun if I do that...
RCS file: /usr/local/src/gitbuild/t/trash
directory.t9200-git-cvsexportcommit/cvsroot/attic_gremlin,v
done
Checking in attic_gremlin;
/usr/local/src/gitbuild/t/trash
directory.t9200-git-cvsexportcommit/cvsroot/attic_gremlin,v  <--
attic_gremlin
initial revision: 1.1
done
[master 20999b1] Added attic_gremlin
 1 files changed, 1 insertions(+), 0 deletions(-)
 create mode 100644 attic_gremlin
\1 better written as $1 at
/usr/local/src/gitbuild/t/../git-cvsexportcommit line 303.
Checking if patch will apply
Huh? Status 'Up-to-date' reported for unexpected file 'attic_gremlin'
Use of uninitialized value in hash element at
/usr/local/src/gitbuild/t/../git-cvsexportcommit line 263.
Use of uninitialized value in hash element at
/usr/local/src/gitbuild/t/../git-cvsexportcommit line 263.
Applying
error: attic_gremlin: already exists in working directory
cannot patch at /usr/local/src/gitbuild/t/../git-cvsexportcommit line 323.
* FAIL 15: re-commit a removed filename which remains in CVS attic
2009/6/12 Robin Rosenberg <robin.rosenberg.lists@dewire.com>:
> CVS har a timestamp in CVS/Entries that has whole second resolution. In
> addition it's built-in method for trying to work around the problem by sleeping
> until the next second wraps around is flaky.  cvsexportcommit already has
> one sleep for this. Maybe it's not enough.

From the comment in cvsexportcommit: # CVS version 1.11.x and 1.12.x sleeps the wrong way to ensure the timestamp # used by CVS and the one set by subsequence file modifications are different. # If they are not different CVS will not detect changes. sleep(1);

I tried changing this to 2 seconds, but I still got a failure on the 7th run. Life's too short to try changing it to 3 seconds in case I'm being bitten by this from the perldoc:

"On some older systems, it may sleep up to a full second less than what you requested, depending on how it counts seconds. Most modern systems always sleep the full amount."

And that's not taking into account sleeps being interrupted due to signals.

Would it be acceptable to simply reorder the tests so this previously unreported error goes away again?

Mike
Nick Woolley· Jul 6, 2009, 13:23 UTC · re: Mike Ralphson · lore

Re: [PATCH] git-cvsexportcommit can't commit files which have been removed from CVS

Mike Ralphson wrote:
Show 12 quoted lines
> debug.out contains the following:
> 
> # before adding file
> cvs status: nothing known about attic_gremlin
> ===================================================================
> File: no file attic_gremlin             Status: Unknown
> 
>    Working revision:    No entry for attic_gremlin
>    Repository revision: No revision control file
> 
> ===================================================================
> File:  space            Status: Needs Patch
This is what I get on this line:
  File:  space           	Status: Up-to-date

Which might explain why it fails the 'up to date check' in your case - it seems that CVS hasn't synchronised the " space" file with the respository? Or is this an artifact created by the race condition Robin mentioned? I gather the problem appears intermittently.

Show 13 quoted lines
> 
> CVS/Entries for the file has
> / space/1.1/Thu Jul  2 12:50:17 2009//
> 
>> You might also try commenting out the following part of my test, so that it
>> should trivially work, and see if there's still an error:
>>
>> #      rm attic_gremlin &&
>> #      cvs -Q rm attic_gremlin &&
>> #      cvs -Q ci -m "removed attic_gremlin"
> 
> It all goes a bit fun if I do that...
> 

Actually, I get the error you do here - my mistake, I should have told you to comment out the whole clause, i.e.:

# (cd "$CVSWORK" && # echo >attic_gremlin && # cvs -Q add attic_gremlin && # cvs -Q ci -m "added attic_gremlin" && # rm attic_gremlin && # cvs -Q rm attic_gremlin && # cvs -Q ci -m "removed attic_gremlin") &&

I've checked this passes for me.
> Would it be acceptable to simply reorder the tests so this previously
> unreported error goes away again?

Speaking personally, I can't see why not, although I don't understand the mechanism of the race condition in question. Wouldn't a race condition like this potentially cause similar errors to crop up all through the test script, whenever git-cvsexportingcommit gets a bogus "Needs Patch" status?

An alternative might be to start my particular test by creating a new CVS repository.

Cheers,
N

ps I've patched git-cvsexportcommit.perl to get rid of the warnings I can also see in your output, and will submit those to this list separately.

← back to recent threads