[darcs-users] [patch232] Accept issue1232: darcs convert fails if... (and 1 more)

[Dino Morelli]

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 <dino at ui3.info>
>>   * Accept issue1232: darcs convert fails if missing _darcs/prefs/prefs in src
>>
>> Tue May  4 21:39:46 EDT 2010  Dino Morelli <dino at ui3.info>
>>   * 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: dino at ui3.info  web: http://ui3.info/d/  irc: dino-
pubkey: http://ui3.info/d/dino-4AA4F02D-pub.gpg  twitter: dino8352