2 patches for repository /home/dino/dev/darcs/trunk:

Tue May  4 18:22:26 EDT 2010  Dino Morelli <dino@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@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.

Attachments

• accept-issue1232_-darcs-convert-fails-if-missing-_darcs_prefs_prefs-in-src.dpatch
• unnamed

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?

Eric

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 :-( 

On Wed, May 05, 2010 at 01:56:52 +0000, Dino Morelli wrote:
> Tue May  4 18:22:26 EDT 2010  Dino Morelli <dino@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@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.

> -         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...

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9

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@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@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@ui3.info  web: http://ui3.info/d/  irc: dino-
pubkey: http://ui3.info/d/dino-4AA4F02D-pub.gpg  twitter: dino8352

Eric Kow <kowey@darcs.net> writes:
> On Wed, May 05, 2010 at 01:56:52 +0000, Dino Morelli wrote:
>> Tue May  4 18:22:26 EDT 2010  Dino Morelli <dino@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@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.
Ack, you don't need that || exit 1, and it's also more customary to write
    test -e ...
instead of
    [ -e

(or, test -f, even)

When you amend the patches as Eric asked (i.e. avoid Accept
issue1232/Resolve issue1232 I think this should go in).

Yours,
   Petr.

2 patches for repository /home/dino/dev/darcs/trunk:

Wed May  5 10:01:11 EDT 2010  Dino Morelli <dino@ui3.info>
  * Extend issue1232: test to account for missing _darcs/prefs/prefs case
  
  Also now sourcing tests/lib.

Wed May  5 10:15:00 EDT 2010  Dino Morelli <dino@ui3.info>
  * Extend issue1232: 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.

Attachments

• extend-issue1232_-test-to-account-for-missing-_darcs_prefs_prefs-case.dpatch
• unnamed

> Wed May  5 10:01:11 EDT 2010  Dino Morelli <dino@ui3.info>
>   * Extend issue1232: test to account for missing _darcs/prefs/prefs case
>   
>   Also now sourcing tests/lib.
> 
> Wed May  5 10:15:00 EDT 2010  Dino Morelli <dino@ui3.info>
>   * Extend issue1232: 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.

These look fine and they'll be going in.

One remark below.

Extend issue1232: test to account for missing _darcs/prefs/prefs case
---------------------------------------------------------------------
> +# Create source repo
> +darcs init --old-fashioned-inventory --repo R
> +
> +# Check that the new repo is d2
> +test -f S/_darcs/hashed_inventory

In case there's any confusion about old-fashioned vs hashed vs
darcs-2 here, check out
http://wiki.darcs.net/RepositoryFormats

There's a slight risk of the script perpetuating such confusion.  You
could have init --hashed and checked for darcs 2 by some other means
(err, show repo?)

It's a minor point of little consequence.

-- 
Eric Kow <http://www.nltg.brighton.ac.uk/home/Eric.Kow>
PGP Key ID: 08AC04F9

This patch bundle (with 2 patches) was just applied to the repository http://darcs.net/.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_.html#bundle-39b200d0cdcb2afa8d4ec2adc0cb871863ad7c77

This patch bundle (with 2 patches) was just applied to the repository http://darcs.net/reviewed.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_reviewed.html#bundle-39b200d0cdcb2afa8d4ec2adc0cb871863ad7c77