On Wed, 5 May 2010, Eric Kow wrote:
Some more admin/training noise.
First of all, I definitely appreciate the effort you're putting into
trying to make our system work! But here I think you're working a
little too hard :-P
I'm doubt it really makes sense to name these patches accept issue1232
and resolve issue 1232 respectively because that creates confusion about
what issue1232 actually is.
So technically the right thing to do would be to create a new ticket,
but YUCK! that's a lot of noise for a minor issue. I think what I would
have happily accepted is a patch saying "Extend issue1232 test to
account for missing _darcs/prefs/prefs case". See what I mean?
Care to tweak?
Makes sense that this should have a different desc. So that's Extend for
both of them instead of Accept and Resolve. Will change.
PS. I worry that the meta noise I make sounds like just being anal
and caring more about Process than Work. Process is *not* the point
here. Or at least, it is only the point to the extent that it's
about trying to figure out what makes things easiest for the group
in the long run. I just hope that all this meta-noise I generate
does not paralyse the team :-(
I understand. In a project of this size and complexity there has to be
structure in how people contribute. I think there should often be
structure in almost any not one-off work we do. I have a process for
personal projects.
On Wed, May 05, 2010 at 01:56:52 +0000, Dino Morelli wrote:
Tue May 4 18:22:26 EDT 2010 Dino Morelli <[email protected]>
* Accept issue1232: darcs convert fails if missing _darcs/prefs/prefs in src
Tue May 4 21:39:46 EDT 2010 Dino Morelli <[email protected]>
* Resolve issue 1232: darcs convert fails if missing _darcs/prefs/prefs
In the earlier fix for this issue, did not consider the possibility that
the prefs file may not be present in the source repo.
+# Check that the new repo is d2
+[ -e S/_darcs/hashed_inventory ] || exit 1
You don't actually need to || exit 1 here
I don't think.
Ah, this is what you were saying, they all run with 'set -e' in effect.
ok
- prefsRelPath Uncachable
+ prefsRelPath Uncachable `catchall` return ()
Looks fine to me. Thanks for fixing this (and apologies for not
catching it the first time!).
The art of doing a good review is not just look at what the patch
says, but think very very very hard about what the patch does NOT
say... and this is where I experienced review fail. Oh well, I
just have to keep on trying and keep on learning...
Thank you
--
Dino Morelli email: [email protected] web: http://ui3.info/d/ irc: dino-
pubkey: http://ui3.info/d/dino-4AA4F02D-pub.gpg twitter: dino8352
_______________________________________________
darcs-users mailing list
[email protected]
http://lists.osuosl.org/mailman/listinfo/darcs-users
