git-cvsimport-3 and incremental imports

I've now spent some time looking at git-cvsimport-3.py from
jc/cvsimport-upgrade and made some progress in making it pass more of
the Git test suite (my work in progress is at [1]).

However, I think there is a fundamental problem with the way it handles
incremental imports and I'm hoping someone with more git-fast-import
experience can point me in the right direction.

Currently, cvsps-3 never writes a "from ..." line in the first commit it
outputs for a branch, even when the output is restricted by date (i.e. a
continuation of a previously imported branch), which results in failure
to update branches since git-fast-import is run without "--force".  If I
make a simple modification so that it does this, it can end up
outputting an empty commit (a duplicate of the current tip commit on the
branch).

Given that the start date for the import is currently just read from
HEAD there is probably scope for this being worse on other branches if
they have more recent commits than the current branch.

I don't think there is any way to solve this without giving cvsps more
information, probably the last commit time for all git branches, but
perhaps I'm missing a fast-import feature that can help solve this
problem.


[1] https://github.com/johnkeeping/git/tree/cvsimport-3


John

John Keeping [off-list ref]:

I don't think there is any way to solve this without giving cvsps more
information, probably the last commit time for all git branches, but
perhaps I'm missing a fast-import feature that can help solve this
problem.

Yes, you are.  The magic incantation is

    from refs/heads/<branch>^0

I've just pushed a cvsps-3.9 with an -i option that generates these at
each branch root.  Combine it with -d and you get incremental
fast-export.

You get to integrate this.  I think the transition strategy Junio
has chosen is seriously mistaken, leading to unnecessary grief for users
who will be fooled into thinking it's OK to still use cvsps-2.x. Because
I do not wish to encourage or endorse this mistake and am tired of arguing
against stubborn determination to do the wrong thing, I am not going to 
sink more effort into the git project's end of the CVS-lifting problem.
There are too many better uses for my time.
-- 
		<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>

Eric S. Raymond wrote:

You get to integrate this.  I think the transition strategy Junio
has chosen is seriously mistaken, leading to unnecessary grief for users
who will be fooled into thinking it's OK to still use cvsps-2.x.

So our choices are:

 a. support current users, offend ESR, don't benefit from ESR
    improvements

 b. give up on current users, please ESR, benefit from ESR
    improvements

 c. some option yet undiscovered

In that case, (c) sounds like our best bet.  Do I understand
correctly?

Sigh,
Jonathan

Eric S. Raymond wrote:

You get to integrate this.  I think the transition strategy Junio
has chosen is seriously mistaken, leading to unnecessary grief for users
who will be fooled into thinking it's OK to still use cvsps-2.

Ah, I missed a detail on first reading.  I think there has been a
small misunderstanding.

Junio proposed a transition strategy, but I don't think it's fair to
say he has chosen it without discussion or is imposing it on you.  If
you or someone else comes up with a better transition strategy, you
are free to completely ignore Junio's proposal.  If the affected users
generally prefer your alternate proposal, in my experience that means
it will be integrated.

Hoping that clarifies a little,
Jonathan

Jonathan Nieder [off-list ref]:

Junio proposed a transition strategy, but I don't think it's fair to
say he has chosen it without discussion or is imposing it on you.

I have said everything I could about the bad effects of encouraging
people to continue to use cvsps-2.x, it didn't budge Junio an
inch, and I'm tired of fighting about it.  Quibbling about the 
semantics of 'impose' will neither change these facts nor make
me any less frustrated with the outcome.

I will continue to do what I can to make cvsps-3.x and cvs-fast-export as
bug-free as possible, given the innate perverseness of CVS.  They
won't be perfect; they will be *better*.
-- 
		<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>

Eric S. Raymond wrote:

I have said everything I could about the bad effects of encouraging
people to continue to use cvsps-2.x, it didn't budge Junio an
inch, and I'm tired of fighting about it.

What I think you misunderstood is that Junio is not the person you
would have needed to convince.  (Nor is it me.)

[...]

I will continue to do what I can to make cvsps-3.x and cvs-fast-export as
bug-free as possible, given the innate perverseness of CVS.

That's all anyone could hope for. :)  Thanks.

Ciao,
Jonathan

On Sun, Jan 20, 2013 at 06:20:08PM -0500, Eric S. Raymond wrote:

John Keeping [off-list ref]:

quoted

I don't think there is any way to solve this without giving cvsps more
information, probably the last commit time for all git branches, but
perhaps I'm missing a fast-import feature that can help solve this
problem.

Yes, you are.  The magic incantation is

    from refs/heads/<branch>^0

I've just pushed a cvsps-3.9 with an -i option that generates these at
each branch root.  Combine it with -d and you get incremental
fast-export.

I don't think this is enough.  I made a very similar change here for
testing (conditional on relative_date_start instead of a new flag) and I
needed this as well in order to prevent empty duplicate commits being
added:

-- >8 --

diff --git a/cvsps.c b/cvsps.c
index fb6a3ad..5771462 100644
--- a/cvsps.c
+++ b/cvsps.c

@@ -1560,7 +1560,7 @@ static bool visible(PatchSet * ps)
  ok:
     //fprintf(stderr, "Time check: %zd %zd %zd\n", restrict_date_start, restrict_date_end, ps->date);
     if (restrict_date_start > 0 &&
-	(ps->date < restrict_date_start ||
+	(ps->date <= restrict_date_start ||
 	 (restrict_date_end > 0 && ps->date > restrict_date_end)))
 	return false;
 
-- 8< --

But this is nothing more than a sticking plaster that happens to do
enough in this particular case - if the Git repository happened to be on
a different branch, the start date would be wrong and too many or too
few commits could be output.  Git doesn't detect that they commits are
identical to some that we already have because we're explicitly telling
it to make a new commit with the specified parent.

You can easily see the breakage by running the tests in the Git tree,
where the CVS revision map tests fail because they end up with duplicate
versions.

You'll need my cvsimport-3 branch to see these failures as it adds the
"git config" support that the tests rely on:

    git://github.com/johnkeeping/git.git cvsimport-3


John

John Keeping [off-list ref]:

But this is nothing more than a sticking plaster that happens to do
enough in this particular case

I'm beginning to think that's the best outcome we ever get in this
problem domain...

                   - if the Git repository happened to be on
a different branch, the start date would be wrong and too many or too
few commits could be output.  Git doesn't detect that they commits are
identical to some that we already have because we're explicitly telling
it to make a new commit with the specified parent.

Then I don't understand the actual failure case.  Either that or you
don't understand the effect of -i. Have you actually experimented with
it?  The reason I suspect you don't understand the feature is that it
shouldn't make any difference to the way -i works which repository branch is
active at the time of the second import.

Here is how I model what is going on:

1. We make commits to multiple branches of a CVS repo up to some given time T.

2. We import it, ending up with a collection of git branches all of which 
   have tip commits dated T or earlier. And *every* commit dated T or earlier
   gets copied over.

3. We make more commits to the same set of branches in CVS.

4. We now run cvsps -d T on the repo. This generates an incremental
   fast-import stream describing all CVS commits *newer* than T (see
   the cvsps manual page).

5. That stream should consist of a set of disconnected branches, each
   (because of -i) beginning with a root commit containing "from
   refs/heads/foo^0" which says to parent the commit on the tip of
   branch foo, whatever that happens to be.  (I don't have to guess
   about this, I tested the feature before shipping.)

6. Now, when git fast-import interprets that stream in the context of
   the repository produced in step 2, for each branch in the
   incremental dump the branch root commit is parented on the tip
   commit of the same branch in the repo.
 
At step 6, it shouldn't matter at all which branch is active, because
where an incremental branch root gets attached has nothing to do with
which branch is active. 

It is sufficient to avoid duplicate commits that cvsps -d 0 -d T and
cvsps -d T run on the same CVS repo operate on *disjoint sets* of CVS
file commits.  I can see this technique possibly getting confused if T
falls in the middle of a changeset where the CVS timestamps for the
file commits are out of order.  But that's the same case that will
fail if we're importing at file-commit granularity, so there's no new
bug here.

Can you explain at what step my logic is incorrect?
-- 
		<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>

On Mon, Jan 21, 2013 at 06:28:53AM -0500, Eric S. Raymond wrote:

John Keeping [off-list ref]:

quoted

But this is nothing more than a sticking plaster that happens to do
enough in this particular case

I'm beginning to think that's the best outcome we ever get in this
problem domain...

I don't think we can ever get a perfect outcome, but it should be
possible to do a little bit better without too much effort.

quoted

                   - if the Git repository happened to be on
a different branch, the start date would be wrong and too many or too
few commits could be output.  Git doesn't detect that they commits are
identical to some that we already have because we're explicitly telling
it to make a new commit with the specified parent.

Then I don't understand the actual failure case.  Either that or you
don't understand the effect of -i. Have you actually experimented with
it?  The reason I suspect you don't understand the feature is that it
shouldn't make any difference to the way -i works which repository branch is
active at the time of the second import.

Here is how I model what is going on:

1. We make commits to multiple branches of a CVS repo up to some given time T.

2. We import it, ending up with a collection of git branches all of which 
   have tip commits dated T or earlier. And *every* commit dated T or earlier
   gets copied over.

3. We make more commits to the same set of branches in CVS.

4. We now run cvsps -d T on the repo. This generates an incremental
   fast-import stream describing all CVS commits *newer* than T (see
   the cvsps manual page).

This is the problem step.  There are two scenarios that have problems:

1. If I create a new development branch in my Git repository and commit
   something to it then git-cvsimport-3 will pass a time to cvsps that
   is newer than the actual time of the last import, so T is wrong.

   It may be possible to fix this case purely in git-cvsimport-3.

2. If the branch I have checked out is not the newest CVS branch, then
   git-cvsimport-3 will pass a value of T that is before the time of the
   last import.  This case is more subtle but it results in unwanted
   duplicate commits since git-fast-import will just do what it's told
   and create the new commits.

   So if we have the following commits:

     commit1 at time 1
     commit2 at time 2
     commit3 at time 3

   and I call "cvsps -d 2 -i" I end up with the series:

     commit1 at time 1
     commit2 at time 2
     commit3 at time 3
     commit2 at time 2 - effectively reverting the previous commit
     commit3 at time 3 - a duplicate
     ... and potentially genuinely new commits

   This is demonstrated by running the Git test t9650.

I also disagree that cvsps outputs commits *newer* than T since it will
also output commits *at* T, which is what I changed with the patch in my
previous message.  This fixes the duplicate commit2 in the series above,
but not the duplicate commit3.

5. That stream should consist of a set of disconnected branches, each
   (because of -i) beginning with a root commit containing "from
   refs/heads/foo^0" which says to parent the commit on the tip of
   branch foo, whatever that happens to be.  (I don't have to guess
   about this, I tested the feature before shipping.)

6. Now, when git fast-import interprets that stream in the context of
   the repository produced in step 2, for each branch in the
   incremental dump the branch root commit is parented on the tip
   commit of the same branch in the repo.
 
At step 6, it shouldn't matter at all which branch is active, because
where an incremental branch root gets attached has nothing to do with
which branch is active. 

It is sufficient to avoid duplicate commits that cvsps -d 0 -d T and
cvsps -d T run on the same CVS repo operate on *disjoint sets* of CVS
file commits.  I can see this technique possibly getting confused if T
falls in the middle of a changeset where the CVS timestamps for the
file commits are out of order.  But that's the same case that will
fail if we're importing at file-commit granularity, so there's no new
bug here.

Can you explain at what step my logic is incorrect?

Your logic is correct - for cvsps - the problem is where T comes from.

Perhaps it is simplest to just save a CVS_LAST_IMPORT_TIME file in
$GIT_DIR and not worry about it any more.


John

John Keeping [off-list ref]:

I also disagree that cvsps outputs commits *newer* than T since it will
also output commits *at* T, which is what I changed with the patch in my
previous message.

Ah.  OK, that is yet another bug inherited from 2.x - the code doesn't
match the documented (and correct) behavior.  Please send me a patch
against the cvsps repo, I'll merge it.

Perhaps it is simplest to just save a CVS_LAST_IMPORT_TIME file in
$GIT_DIR and not worry about it any more.

Yes, I think you're right. Trying to carry that information in-band would
probably doom us to all sorts of bug-prone complications.

Thanks for the good analysis.  I wish everybody I had to chase bugs with
could explain them with such clarity and concision.

Sigh. Now I have to figure out if cvsps's behavior can be rescued in Chris
Rorvick's recently-discovered failure case. I'm not optimistic.
-- 
		<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>

On Mon, Jan 21, 2013 at 07:43:40AM -0500, Eric S. Raymond wrote:

John Keeping [off-list ref]:

quoted

I also disagree that cvsps outputs commits *newer* than T since it will
also output commits *at* T, which is what I changed with the patch in my
previous message.

Ah.  OK, that is yet another bug inherited from 2.x - the code doesn't
match the documented (and correct) behavior.  Please send me a patch
against the cvsps repo, I'll merge it.

Should now be in your inbox.

quoted

Perhaps it is simplest to just save a CVS_LAST_IMPORT_TIME file in
$GIT_DIR and not worry about it any more.

Yes, I think you're right. Trying to carry that information in-band would
probably doom us to all sorts of bug-prone complications.

I think the only way to do it without needing to save local state in the
Git repository would be to teach cvsps to read a table of refs and times
from its stdin so that we could do something like:

    git for-each-ref --format='%(refname)%09%(*authordate:raw)' refs/heads/ |
    cvsps -i --branch-times-from-stdin |
    git fast-import

Then cvsps could create a hash table from this and use that to decide
whether a patch set is interesting or not.


John

John Keeping [off-list ref]:

quoted

Ah.  OK, that is yet another bug inherited from 2.x - the code doesn't
match the documented (and correct) behavior.  Please send me a patch
against the cvsps repo, I'll merge it.

Should now be in your inbox.

Received, merged, tested, and cvsps-3.10 has shipped.
 

I think the only way to do it without needing to save local state in the
Git repository would be to teach cvsps to read a table of refs and times
from its stdin so that we could do something like:

    git for-each-ref --format='%(refname)%09%(*authordate:raw)' refs/heads/ |
    cvsps -i --branch-times-from-stdin |
    git fast-import

Then cvsps could create a hash table from this and use that to decide
whether a patch set is interesting or not.

Agreed.  I considered implementing something quite this before thinking of
the ^0 hack.  But an out-of-band timestamp file is much simpler.
-- 
		<a href="http://www.catb.org/~esr/">Eric S. Raymond</a>