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

Fri Apr 16 22:32:37 EDT 2010  Dino Morelli <dino@ui3.info>
  * Accept issue 1232: darcs convert forgets _darcs/prefs/prefs

Fri Apr 16 22:34:03 EDT 2010  Dino Morelli <dino@ui3.info>
  * resolve issue1232: darcs convert forgets _darcs/prefs/prefs


* What your patch changes

   issue 1232 darcs convert forgets _darcs/prefs/prefs

* Why your patch changes it

   Longstanding issue in the convert feature. Discussed as worth fixing in messages.

* How you know your changes are correct

   Tested with new testcase script:
      tests/issue1232_convert_forgets_prefs.sh

* If you have future work in this line

   No future work

Attachments

• accept-issue-1232_-darcs-convert-forgets-_darcs_prefs_prefs.dpatch
• unnamed

Welcome to a world of Darcs hacking, Dino :-)

Accept issue 1232: darcs convert forgets _darcs/prefs/prefs
-----------------------------------------------------------
> +# Create source repo
> +darcs init --old-fashioned-inventory --repo R
> +
> +# Do something easy that will create a prefs file
> +darcs setpref test 'true' --repo R
> +
> +# Perform the d1 to d2 conversion
> +echo "I understand the consequences of my action" | darcs convert R S
> +
> +# Check that the prefs file was copied over
> +prefsPath="_darcs/prefs/prefs"
> +diff R/$prefsPath S/$prefsPath

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.

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.

This is going to be really nit-picky and a lot of it is just going to be
subject to taste.  Hopefully you'll be able to pick out the core message.

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.
 
> Fri Apr 16 22:34:03 EDT 2010  Dino Morelli <dino@ui3.info>
>   * resolve issue1232: darcs convert forgets _darcs/prefs/prefs
> 
> 
> * What your patch changes
> 
>    issue 1232 darcs convert forgets _darcs/prefs/prefs
>
> * Why your patch changes it
> 
>    Longstanding issue in the convert feature. Discussed as worth fixing in messages.

OK some really minor points to start off.

m1) We've got some excessive whitespace here.  On the one hand, this may be a
    superficial subject-to-taste kind of comment.  On the other hand, I think
    it points back to the key idea of maximising usefulness.  You want to have
    *enough* whitespace to help people understand your patch comment, but no
    more!

m2) There's no need to make a form out of the suggestions.  Simplify, simplify!
    Reduce clutter.

m3) Eliminate redundancy.  Your short patch name told us what the patch
    does already, why tell it to us again?

And now a more important point: what we really mean by "what your patch
changes" and "why your patch changes it" is what you have changed in the code
and why.  Here you gotta put yourself in the reviewer's perspective.  They've
got a big blob of code ahead to read with lots of little changes. To make it
easier on your reviewer, you want to give them a sort of roadmap to the work,
to make it easier to understand why specific bits of the code have changed in a
particular way.

Have a look at this patch (using darcs changes -v):

Sat Aug 29 08:27:33 BST 2009  Judah Jacobson <judah.jacobson@gmail.com>
  * Resolve issue1578: Don't put newlines in the Haskeline prompts.

  Haskeline doesn't expect to get control characters in its prompt.
  The fix is to manually print all but the last line of a prompt message
  separately, and then pass the last line as the Haskeline prompt.
  
  So far we've only seen this cause a problem when mark-conflicts is run in
  the emacs shell (see the issue for more information).

I think this sort of captures the idea of what we're looking for.

 Too high level: fix issue1578 (you already told us that)

 Too low level: replace promptYorn with putStrLn; promptYorn
                (we can see that by looking at the code)

 Just right: manually print all but the last line of the prompt
             message (ah that's what's changed, and the "Haskeline
             doesn't expect to get control characters" tells us
             why)

It's a fine art!  And don't take this level of nitpickiness the wrong way.  I'm
not very good at this sort of thing myself, but I'm trying to point to the
ideal anyway :-)
 
> * How you know your changes are correct
> 
>    Tested with new testcase script:
>       tests/issue1232_convert_forgets_prefs.sh
> 
> * If you have future work in this line
> 
>    No future work

Mostly superfluous.  The last point was really to say that it probably makes
sense to give the reader an idea what work is going to be built on this patch.
So if you're refactoring the SelectChanges module, you would say something
like "this will allow us in the future to break the Darcs library into a core
and command level pieces"... [it's also good not to over-guess this, the main
idea is just to help people see the story emerging from your line of work].

The default case is that there is no future work, so no need to say there is
none.

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

[Then again, sometimes, when I'm changing a line, I also will sneak in some
style changes along the way, for example

- putStrLn ("foo"++bar++"wibble")
+ putStrLn ("foo" ++ quux ++ "wibble")

There is partly because (a) I'm being a hypocrite and (b) what I'm doing is
weighing the trade-off between making the change here and now because it's
obvious anyway and introducing another patch.   It's all about tradeoffs
and in the case of the indentation stuff above, you probably want to err on
the side of minimal changes]

> +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)

> +      -- Copy over the prefs file
> +      let prefsRelPath = darcsdir </> "prefs" </> "prefs"
> +      copyFileOrUrl [NoLinks] (repodir </> prefsRelPath)
> +         prefsRelPath Uncachable

Finally, is it really the right approach to just copy the prefs file?

What if I accidentally edit it by hand in the source repository.  Won't I get
the wrong pref as a result?

You *could* argue that this is what darcs get does anyway, and maybe you'd be
right, but I would kind of hope that darcs convert (doing something that people
are quite sensitive about anyway) would set itself to a higher standard by
trying to be more scrupulous about it.  It could for example, grab the change
from the very last setpref patch.  Or it could just update _darcs/prefs/prefs
anytime it sees a setpref patch.

Anyway, I think the latter two changes may be a lot trickier for a new Darcs
hacker to take on, so I think if would amend your patch with a more concise
explanation and with more minimal changes, I'll just apply it and leave it at
that.

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

This was obsoleted by patch@X+1@

Handy tip for the darcs.net patch tracker: you can submit amendments to
patches by just saying

   darcs send --subject '[patch@X@]'

Doing so saves you the trouble of keeping track of multiple patch
objects and
also makes the review history easier to follow.  For more patch tracker
hints,
see http://wiki.darcs.net/Development/PatchTracker.