threads / discuss / 10648
Subject: Warning: cvsexportcommit considered dangerous
Johannes SchindelinHi,
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
Steffen ProhaskaRe: Warning: cvsexportcommit considered dangerous
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
Johannes SchindelinRe: Warning: cvsexportcommit considered dangerous
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
Robin RosenbergRe: Warning: cvsexportcommit considered dangerous
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
Johannes SchindelinRe: Warning: cvsexportcommit considered dangerous
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
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.