[darcs-users] darcs patch: simplify cleanup in overriding-defaults ... (and 5 more)
kowey at darcs.net
Thu Mar 19 08:26:45 UTC 2009
Thanks for the review!
On Thu, Mar 19, 2009 at 18:11:55 +1100, Trent W. Buck wrote:
> BTW, I would prefer that David start patch comments with a capital
> letter, particularly as the final period strongly implies that they are
> sentences (and not sentence fragments (clauses), as some other
> contributors seem to prefer).
Yeah, but since David has left the project and is really just working on
his personal version of darcs (though making it public), and we are the
ones choosing to pull from it, there is not much I can say here. :-) We
could choose not to pull David-patches on stylistic grounds or we could
just accept this less than preferable style for his patches. Of course,
in the long run, of course, it would be good to see David returning to
darcs.net, at which point we can discuss this.
> Eric Kow <kowey at darcs.net> writes:
> > simplify cleanup in overriding-defaults test.
> > ---------------------------------------------
> It looks like this test was doing things to $HOME/.darcs and then using
> traps to clean up after itself, and the new patch changes thing to
> simply redefine $HOME (which is a big improvement). +1 for this.
Ok, I'll apply this one.
> > -rm -rf temp1 temp2
> I don't understand why it is "clean" to leave temp directories lying
> around, and not to remove them if another script left them lying
> around. See next comment.
I'm not going to apply this one, then. I suspect there is a deeper
context behind it, which is his moves to make the test suite work with
> Also, this conflicts with another patch, which AIUI is Bad Juju.
Conflicts are fine. It's long-lived conflicts and branches which have
caused us some pain in the past. I don't think we should run away from
conflicts, just not actively court them. Also, the main reason why I
tend to dislike having conflicts is because having to merge them leaves
a lot of room for error (so more "social" reasons than technical ones,
although this straddles the boundary a bit)
> This is wrong. These rm -rf's are very important, because if the
> previous test failed or was interrupted, then temp/ will already exist,
> and the current test will spuriously fail (because "mkdir temp" will
> abort) -- or worse, they will give very confusing errors if "mkdir -p
> temp" is used.
Not applied, then (at least not until somebody gets around to reviewing
David's franchise-based shell harness and seeing what interesting bits
we can apply to Distribution.ShellHarness).
> For a proper solution, see http://bugs.darcs.net/issue1283, "run tests
> in separate subdirs".
> > chmod 0000 no_perms.txt
> > # surely there is an easier way to write this 'not'
> > # i'm think of the 'finally' in try/catch/finally
> > -dacrs add no_perms.txt 2> log && (chmod 0755 no_perms.txt ; exit 1) || chmod 0755 no_perms.txt
> > +darcs add no_perms.txt 2> log && (chmod 0755 no_perms.txt ; exit 1)
> > +chmod 0755 no_perms.txt
> > +cat log
> > grep -i 'permission denied' log
> A more meaningful patch description would have helped me understand
> this. What happens is that the "chmod 0000" creates a file that nobody
> can touch. The later "chmod 0755" is necessary regardless of whether
> the script fails, because otherwise "make clean" will not be able to
> delete that file.
> The sh-ly way of handling this is to trap exit and error signals, such
> that no matter how the script exits, chmod is run.
So I'll not apply this and ask for a patch instead?
> I can't comment on this one.
Thanks for covering the ones you could. I'm trying to keep them
in thematic clumps, but not always succeeding.
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Size: 197 bytes
Desc: Digital signature
More information about the darcs-users