Bug in git-cvsexportcommit: can't commit files which have been removed from CVS

Bug in git-cvsexportcommit: can't commit files which have been removed from CVS

[Nick Woolley]

Hi,

I think I've discovered a bug in git-cvsexportcommit: where a file has been
removed from a CVS repository, it linger in the Attic and CVS perversely reports
them as 'no file X' with status 'Up-to-date'.  git-cvsexportcommit then
prevents files from being added with the same name.

I have a patch against the current version of git's repository which seems to
fix the problem.  This is actually three commits:
 - the fix,
 - an extension to t9200-git-cvsexportcommit.sh to test the fix
 - EOL whitespace-removal

I'm hesitant to send all three as separate emails, even though the
SubmittingPatches document seems to imply I should - should I squash them into
one commit?


Anyway - to replicate the issue, create a CVS repository, add a file "x", then
remove it.

Then create a git repository tracking this CVS repository using git-cvsimport.
Pull the changes from CVS, then try adding a file "x", and exporting it back to
CVS.  When I do that I get this error:

  File x 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
  Exiting: your CVS tree is not clean for this merge. at
  /usr/lib/git-core/git-cvsexportcommit line 275.

When comitting, I also sometimes get warnings like this:

  Huh? Status reported for unexpected file 'no file y'

These seem to be ignorable, but I think they also result from the above issue.


Cheers,

Nick

Re: Bug in git-cvsexportcommit: can't commit files which have been removed from CVS

[Jeff King]

On Thu, May 28, 2009 at 05:53:24PM +0100, Nick Woolley wrote:

> I have a patch against the current version of git's repository which
> seems to fix the problem.  This is actually three commits:
>  - the fix,
>  - an extension to t9200-git-cvsexportcommit.sh to test the fix
>  - EOL whitespace-removal
> 
> I'm hesitant to send all three as separate emails, even though the
> SubmittingPatches document seems to imply I should - should I squash
> them into one commit?

Don't worry about sending multiple messages to the list. It is the
normal behavior here. But you may want to collapse it somewhat:

  1. If your whitespace removal is a cleanup in nearby code, then that
     should probably come as the first patch.

  2. Your fix and the test extension should probably come in the same
     patch (we do sometimes do tests separately beforehand, marking them
     to expect failure, but that is usually only because nobody has a
     fix when the test is written :) ).

     If the whitespace removal is for the lines in your actual fix,
     then that should just be squashed in. There is no point showing us
     your broken-styled code to review, only to fix it in the very next
     commit.

So I would expect either a single patch (with cleaned-up fix and tests)
or a two-patch series (cleanups in the area, followed by your fix and
tests).

-Peff