Warning: cvsexportcommit considered dangerous

Warning: cvsexportcommit considered dangerous

[Johannes Schindelin]

Hi,

ever since the up-to-date check was changed to use just one call to "cvs 
status", a bug was present.  Now cvsexportcommit expects "cvs status" to 
return the results in the same order as the file names were passed.

This is not true, as I had to realise with one of my projects on 
sourceforge.

Since time is so scarce on my side, I will not have time to fix this bug, 
but will instead return to my old "commit by hand" procedure.

Ciao,
Dscho

Re: Warning: cvsexportcommit considered dangerous

[Steffen Prohaska]

On Nov 4, 2007, at 5:41 PM, Johannes Schindelin wrote:

> ever since the up-to-date check was changed to use just one call to  
> "cvs
> status", a bug was present.  Now cvsexportcommit expects "cvs  
> status" to
> return the results in the same order as the file names were passed.
>
> This is not true, as I had to realise with one of my projects on
> sourceforge.
>
> Since time is so scarce on my side, I will not have time to fix  
> this bug,
> but will instead return to my old "commit by hand" procedure.

I introduced this 'optimization', which turned out to be a bug.
So, I feel responsible. Sorry for the trouble.

In August this was already recognized and a patch submitted:

http://marc.info/?t=118718458000004&r=1&w=2

I do not know why it wasn't applied. I forgot re-checking after my
vacation.

I put all on CC who replied to the patch in August.

	Steffen

Re: Warning: cvsexportcommit considered dangerous

[Johannes Schindelin]

Hi,

On Sun, 4 Nov 2007, Steffen Prohaska wrote:

> On Nov 4, 2007, at 5:41 PM, Johannes Schindelin wrote:
> 
> > ever since the up-to-date check was changed to use just one call to 
> > "cvs status", a bug was present.  Now cvsexportcommit expects "cvs 
> > status" to return the results in the same order as the file names were 
> > passed.
> > 
> > This is not true, as I had to realise with one of my projects on 
> > sourceforge.
> > 
> > Since time is so scarce on my side, I will not have time to fix this 
> > bug, but will instead return to my old "commit by hand" procedure.
> 
> I introduced this 'optimization', which turned out to be a bug. So, I 
> feel responsible. Sorry for the trouble.
> 
> In August this was already recognized and a patch submitted:
> 
> http://marc.info/?t=118718458000004&r=1&w=2
> 
> I do not know why it wasn't applied. I forgot re-checking after my 
> vacation.

It slipped by me, because of holiday, too.  (I was on my well needed 
holiday then.)

But that patch really seems like a step back to me.  The line "File: ... 
Status: ..." should be parsable enough to fix the bug properly, instead of 
undoing the optimisation.

AFAICS Robin replied with a "let's see if a proper fix materialises", and 
I kind of hope that it will materialise soon.

Ciao,
Dscho

Re: Warning: cvsexportcommit considered dangerous

[Robin Rosenberg]

söndag 04 november 2007 skrev Johannes Schindelin:
> Hi,
> 
> On Sun, 4 Nov 2007, Steffen Prohaska wrote:
> 
> > On Nov 4, 2007, at 5:41 PM, Johannes Schindelin wrote:
> > 
> > > ever since the up-to-date check was changed to use just one call to 
> > > "cvs status", a bug was present.  Now cvsexportcommit expects "cvs 
> > > status" to return the results in the same order as the file names were 
> > > passed.
> > > 
> > > This is not true, as I had to realise with one of my projects on 
> > > sourceforge.
> > > 
> > > Since time is so scarce on my side, I will not have time to fix this 
> > > bug, but will instead return to my old "commit by hand" procedure.
> > 
> > I introduced this 'optimization', which turned out to be a bug. So, I 
> > feel responsible. Sorry for the trouble.
> > 
> > In August this was already recognized and a patch submitted:
> > 
> > http://marc.info/?t=118718458000004&r=1&w=2
> > 
> > I do not know why it wasn't applied. I forgot re-checking after my 
> > vacation.
> 
> It slipped by me, because of holiday, too.  (I was on my well needed 
> holiday then.)
> 
> But that patch really seems like a step back to me.  The line "File: ... 
> Status: ..." should be parsable enough to fix the bug properly, instead of 
> undoing the optimisation.
Unfortunately it's not that easy to parse. It *can* be done by looking at the
repository path, and the CVS/Root etc, but it's not nice. 

> 
> AFAICS Robin replied with a "let's see if a proper fix materialises", and 
> I kind of hope that it will materialise soon.

Still hoping :). BTW, wouldn't this err on the right side anyway, i.e. if an
existing file was not up to date and was wrongly thought to not exist or a new 
file was thought to be up-to-date, I would get an error and would not be able
to commit. I've never seen it though and I always have a clean CVS checkout
so the potential bug seems unlikely to me.

The command I always use is.

	git cvsexportcommit -u -c -w /my/cvs/checkout

Never bitten me yet (touch wood).

My real worry is on the other side, with bad conversion from CVS to git.

-- robin

Re: Warning: cvsexportcommit considered dangerous

[Johannes Schindelin]

Hi,

On Mon, 5 Nov 2007, Robin Rosenberg wrote:

> BTW, wouldn't this err on the right side anyway, i.e. if an existing 
> file was not up to date and was wrongly thought to not exist or a new 
> file was thought to be up-to-date, I would get an error and would not be 
> able to commit. I've never seen it though and I always have a clean CVS 
> checkout so the potential bug seems unlikely to me.

The problem is that it can err.  For example when I have new files, it 
says that the files were already added by someone else.  And then it 
refuses to do anything.

Ciao,
Dscho

Re: Warning: cvsexportcommit considered dangerous

[Alex Bennee]

On Mon, 2007-11-05 at 00:05 +0100, Robin Rosenberg wrote:
> söndag 04 november 2007 skrev Johannes Schindelin:
> > Hi,
> > On Sun, 4 Nov 2007, Steffen Prohaska wrote:
> > > On Nov 4, 2007, at 5:41 PM, Johannes Schindelin wrote:
> > > > ever since the up-to-date check was changed to use just one call to 
> > > > "cvs status", a bug was present.  Now cvsexportcommit expects "cvs 
> > > > status" to return the results in the same order as the file names were 
> > > > passed.
> > > I do not know why it wasn't applied. I forgot re-checking after my 
> > > vacation.

I think at the time it was felt the speed hit was too great on large
trees. Although my view still is we should always go for correctness
over speed.

> > It slipped by me, because of holiday, too.  (I was on my well needed 
> > holiday then.)
> > 
> > But that patch really seems like a step back to me.  The line "File: ... 
> > Status: ..." should be parsable enough to fix the bug properly, instead of 
> > undoing the optimisation.
> Unfortunately it's not that easy to parse. It *can* be done by looking at the
> repository path, and the CVS/Root etc, but it's not nice. 

I also worry about corner cases in parsing code, especially if you have
to start poking around in CVS/Root files. Another (ugly) method could be
a two pass attempt, falling back to an individual status call if the
"optimized" version isn't sure.

> > AFAICS Robin replied with a "let's see if a proper fix materialises", and 
> > I kind of hope that it will materialise soon.

I've not had another go at fixing this mainly due to being too busy. The
current patch works for me.

> Still hoping :). BTW, wouldn't this err on the right side anyway, i.e. if an
> existing file was not up to date and was wrongly thought to not exist or a new 
> file was thought to be up-to-date, I would get an error and would not be able
> to commit. I've never seen it though and I always have a clean CVS checkout
> so the potential bug seems unlikely to me.

It's not just new files that can break (btw another fix has gone in to
ensure directories are added to CVS trees in order). 

-- 
Alex, homepage: http://www.bennee.com/~alex/
Nothing in life is to be feared. It is only to be understood.