git-cvsimport-3 and incremental imports

git-cvsimport-3 and incremental imports

[John Keeping]

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

Re: git-cvsimport-3 and incremental imports

[Eric S. Raymond]

John Keeping <john@keeping.me.uk>:
> 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>

Re: git-cvsimport-3 and incremental imports

[Jonathan Nieder]

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

Re: git-cvsimport-3 and incremental imports

[Jonathan Nieder]

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

Re: git-cvsimport-3 and incremental imports

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> 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

Isn't (c) is to just build on cvsimport-2 and cvsimport-3 combo?

If Eric does not want to get involved in cvsimport-2 (and cvsps2),
that is perfectly fine.  We have the beginning of cvsimport-3 code
that was released with Sign-off, and as long as cvsps3 produces a
"more corrrect" output stream (either traditional cvsps2 style or as
a fast-import stream) than cvsps2 conversion describes, cvsimport-3
can get the full benefit of his work, no?

I do not think that Eric will be the only person who understands
cvsps3 output who can fix bugs that may potentially still exist in
cvsimport-3 code [*1*].

We live in real world, and distro inertia makes it likely that a new
version of git that does not work for people who have been happy
with cvsps2 will get resistance from them, when they do not trust
the 0.x releases of a new piece of software "cvsps3" that happens to
have a name similar to what they have been shipping, i.e. "cvsps2".
Shipping such a crippled Git will not be an effective way to promote
adoption of "cvsps3".

[Footnote]

*1* This of course assumes Eric's two major claims are true, that
is, (1) cvsimport-3 is already mature and better than cvsimport-2,
and (2) the code is so simple that it does not even need its own
tests as long as the front-end conversion that happens at the cvsps3
end is tested well.  Choosing b. will have to assume them to be true
anyway, so I do not think it is a big loss to rely on the same
assumptions and choose b. from our point of view.

Re: git-cvsimport-3 and incremental imports

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> ..., so I do not think it is a big loss to rely on the same
> assumptions and choose b. from our point of view.

Of course the last sentence is a typo: "choose c." is what I meant.

Re: git-cvsimport-3 and incremental imports

[Eric S. Raymond]

Jonathan Nieder <jrnieder@gmail.com>:
> 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>

Re: git-cvsimport-3 and incremental imports

[Jonathan Nieder]

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

Re: git-cvsimport-3 and incremental imports

[Junio C Hamano]

"Eric S. Raymond" <esr@thyrsus.com> writes:

> Jonathan Nieder <jrnieder@gmail.com>:
>> 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 am not quite sure if I follow you.

The primary thing I am aiming for is to rob an excuse from people
and from their distros to block newer Git and cvsps3.  I've already
cited a few URLs where we can see cvsps3 is blocked with an excuse
"it does not work with Git yet".

If we ship new version of Git that only works with cvsps3, we will
end up giving an excuse "this version of Git no longer works with
cvsps2" for distro packagers whose cvsps distro maintainer is slower
than others to pick up cvsps3 for whatever reason.

You may care deeply about CVS conversion part of the system, but you
need to realize that majority of Git users do not care a whit about
cvsimport. I do not want to give those distros that ship stale cvsps
an excuse to pin their users at an old version of Git.

By shipping both cvsimport-2 and cvsimport-3, we will rob from these
distros the excuse to block shipping the version of Git that does
support cvsps3 output.  They can choose to update cvsps to 3.x and
disable cvsimport-2 altogether at the build time, or if their cvsps
distro maintainer is slow, they can ship both cvsimport-2 and -3 and
let the user install cvsps 3.x under their $HOME if they want.

If you want to abandon cvsps2 users, that is perfectly fine by me.
As long as cvsps3 and cvsimport-3 combo works, Git before this
series will have a _working_ cvsimport as far as I am concerned.

In other words, I am fine to add a paragraph at the top of the
Release Notes saying that

 (1) cvsps2 is no longer maintained;

 (2) using cvsps3 is the future direction for the users; and

 (3) if their distro is slow to adopt cvsps3, however, cvsimport can
     still be used with cvsps2, but bugs in it or cvsps2 are not
     expected to be fixed.

to nudge distros to adopt cvsps3.

> 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*.

Yes, that is what we want.

Re: git-cvsimport-3 and incremental imports

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> If you want to abandon cvsps2 users, that is perfectly fine by me.
> As long as cvsps3 and cvsimport-3 combo works, Git before this
> series will have a _working_ cvsimport as far as I am concerned.

The above obviously is "Git _after_ this series".

Git before this series that only has cvsps2 support may be broken
and Git after this series, when used with cvsps2, may be broken, but
is broken the same way as before, so it is not a net loss.

The users of distros that are slow to update cvsps can still use
cvsimport-3 with cvsps3 that is manually installed, and the users of
distros that ship cvsps3 will use cvsimport-3 and they can migrate
away from cvsps2's breakage.

Re: git-cvsimport-3 and incremental imports

[John Keeping]

On Sun, Jan 20, 2013 at 06:20:08PM -0500, Eric S. Raymond wrote:
> John Keeping <john@keeping.me.uk>:
>> 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

Re: git-cvsimport-3 and incremental imports

[Eric S. Raymond]

John Keeping <john@keeping.me.uk>:
> 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>

Re: git-cvsimport-3 and incremental imports

[John Keeping]

On Mon, Jan 21, 2013 at 06:28:53AM -0500, Eric S. Raymond wrote:
> John Keeping <john@keeping.me.uk>:
>> 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.

>>                    - 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

Re: git-cvsimport-3 and incremental imports

[Eric S. Raymond]

John Keeping <john@keeping.me.uk>:
> 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>

Re: git-cvsimport-3 and incremental imports

[John Keeping]

On Mon, Jan 21, 2013 at 07:43:40AM -0500, Eric S. Raymond wrote:
> John Keeping <john@keeping.me.uk>:
>> 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.

> > 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

Re: git-cvsimport-3 and incremental imports

[Eric S. Raymond]

John Keeping <john@keeping.me.uk>:
> > 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>