Re: [GUILT PATCH 0/4] Add guards to guilt

From: Josef Sipek <jsipek@fsl.cs.sunysb.edu>
Date: Jul 30, 2007, 19:20 UTC
Message-ID: <20070730192027.GB17253@filer.fsl.cs.sunysb.edu>
In-Reply-To: <873az6cmve.fsf@hubert.paunchy.net>

On Mon, Jul 30, 2007 at 01:32:53AM -0700, Eric Lesh wrote:

Show 12 quoted lines

> Josef Sipek <jsipek@fsl.cs.sunysb.edu> writes:
> 
> >
> > I know I've been bad about forcing even myself to write new regression
> > tests. Your patches modify things significantly enough, that I'd like to see
> > some regression tests cases to make sure that user's data is not eaten
> > (e.g., a bug in the guard setting code could blow away the series file =>
> > very bad).
> >
> 
> Yeah, I'll try and make one.  070-guards.sh to test guilt-guard and
> guilt-select plus pushing and popping?

Sounds good.

> There's also sed -i in a few places. For integrity purposes, are a cp
> and sed better?

I like the fact that sed -i makes the code cleaner, BUT I don't want the users to come after me if they patches disappear. Perhaps having a wrapper (a function) for sed that's nicely paranoid and handles errors as well as possible would be in order:

safe_sed [<sed options...>]

Show 6 quoted lines

> > Is this a problem with other projects' implementations of guards as well?
> > Perhaps printing a warning if a new guard is set when there are applied
> > patches would be in order?
> >
> 
> Yeah, they have this problem too,

Good :)

> but tell you so when you select, so guilt should too.

Agreed.

> Mercurial also has two options which do the popping and reapplying for
> you, which I'll try and implement also.

As in:

t=`guilt-top` guilt-pop -a guilt-push "$t"

? Beware that "$t" might not be in the new guarded series.

> Thanks a lot for the review.  Things were pretty ugly, but with your
> help it should look much better.

Nah...just a few nit-picks, that's all :)

Josef 'Jeff' Sipek.

-- 
I think there is a world market for maybe five computers.
		- Thomas Watson, chairman of IBM, 1943.