[darcs-users] [patch210] Accept issue 1232: darcs convert forgets... (and 1 more)

Dino Morelli dino at ui3.info
Sat Apr 17 15:27:53 UTC 2010


On Sat, 17 Apr 2010, Eric Kow wrote:

> Welcome to a world of Darcs hacking, Dino :-)
>
> Accept issue 1232: darcs convert forgets _darcs/prefs/prefs
> -----------------------------------------------------------
..
>
> Looks fine to me.  I'd like to apply this right away, but it's marked as
> a passing test so I'd need to apply the fix first.
>
> Generally speaking, it's nice to submit first the failing test and then
> the bugfix patch which can rename the failing test accordingly.  This
> isn't really a rule (especially because we break it too); the main idea
> is just to help things along by making it possible to apply your patches
> piecemeal.
>

Got it, I should have recorded failing-issue* first and then recorded
a mv with the Resolve patches.


> resolve issue1232: darcs convert forgets _darcs/prefs/prefs
> -----------------------------------------------------------
> I see you've read the Developers' Getting Started guide (thanks!)
> Let's tune the patch comment a bit.
..
> The main point is to write patch comments in such a way that they
> will be useful in the future when you're looking back in the history.
>

I didn't realize that this list of questions was going to become the
long desc. Will rewrite this.


>>  import Darcs.Hopefully ( PatchInfoAnd, n2pia, info, hopefully )
>>  import Darcs.Commands ( DarcsCommand(..), nodefaults, putInfo, putVerbose )
>> -import Darcs.Arguments ( DarcsFlag( AllowConflicts, NewRepo,
>> -                                    SetScriptsExecutable, UseFormat2, NoUpdateWorking),
>> -                        reponame,
>> -                        setScriptsExecutableOption,
>> -                        networkOptions )
>> +import Darcs.Arguments
>> +   ( DarcsFlag
>> +      ( AllowConflicts, NewRepo, SetScriptsExecutable, UseFormat2
>> +      , NoUpdateWorking, NoLinks
>> +      )
>> +   , reponame
>> +   , setScriptsExecutableOption
>> +   , networkOptions
>> +   )
>
> Avoid making stylistic changes that are mixed in with the core patch.  The
> problem is that doing so creates a spurious diff that the reviewer has to wade
> through and think about.  Did anything really change here?
>
> Oh actually, I think you've added the NoLinks flag, but I can't tell because
> there's all these unrelated changes around it.
>

Understood. Will separate that into the small NoLinks addition and a
stylistic patch (which may not be applied, at the discretion of reviewers)


>> +import System.FilePath
>
> We have a policy of using System.FilePath.Posix to make sure that Darcs
> patch handling is predictable.
>
> (This is something David spotted in one of my patches; we had a little
> debate on it and I decided that David was right and we should play it
> safe)
>

Ok. I'll add this info to my darcs hacking notes


There is enough that needs to be changed here that I would like to redo
the patches altogether, including the test script. Will send it all
again shortly.

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


More information about the darcs-users mailing list