This patch might be controversial due to the nature of touching all
the commands and DarcsFlag.  The refactor itself is on the order of
'trivial' but it's a change that bumps up against our long term
goals.

I was trying to understand the code for Annotate and I noticed there
was an opportunity to be correct-by-construction instead of
correct-by-convension.

Ganesh and I discussed the approach to this patch on IRC and we both
agreed that functions like fixFilePathOrStd do not need [DarcsFlag]
and shouldn't receive it as a parameter.  Instead they should accept
the thing they really need (in this case AbsolutePath), and use it
directly.  I refer to this as the Law of Demeter
(http://en.wikipedia.org/wiki/Law_of_Demeter), which is typically about
object oriented programming, but I don't see any reason why it doesn't
apply to other paradigms.  The intuition in the context of Haskell
programming is that you should pass data to functions that they
will work with; instead of data from which they might derive what they
want to work with.  In this specific case, instead of passing
[DarcsFlag] so that the function can search for and extract a
FixFilePath and then further extract an AbsolutePath, just pass the
AbsolutePath and let the caller figure out how to provide that
AbsolutePath.  This enhances the decoupling of the functions in the
program.

The IRC dicussion starts somewhere around here:
  http://irclog.perlgeek.de/darcs/2010-08-22#i_2733160

Ganesh and I disagreed about the right way to get the parameters where
they need to be.  There are about 40 commands in src/Darcs/Commands.
Of those 19 actually use the FixFilePath option that is passed to all
commands.  Ganesh feels that I should modify just those 19 commands to
look at [DarcsFlag] and extract FixFilePath and transform it to the
paths they need.  This would require handling the case when it's
missing in each of the those commands.  Ganesh thinks I should follow
the example set by the function diffingOpts.  And I could have done
that using the extractFixFilePaths function that I removed.  My
reasons follow.

I opted to push the work up one layer for the following reasons:
  * Right now, all commands technically have access to FixFilePath,
  and I didn't want to address that in this refactor.
  * I'm trying to reduce the number of unnecessary partial functions.
  Handling the FixFilePath in each of the commands that use it would
  require the caller of those 19 commands to do the right thing
  (meaning, pass a FixFilePath flag).  Currently that's not many
  places, but it's one more implicit detail to get right for people
  using libdarcs or when commands invoke other commands.
  * By making these paths non-optional parameters the commands that
  need them can count on them being there and not have to look them up
  and do error handling when they're missing.  This error handling
  would be duplicated in about 19 places and never be executed
  (because we always pass a FixFilePath, only developers would
  trigger it).
  * Going this route, we actually have one less data constructor in
  DarcsFlag.  Yay!
  * About half the commands need these paths, and they represent
  read-only application state (ie., execution environment).  In my
  opinion, this is a case where exposing the info to all the commands
  is relatively harmless, althought I appreciate Ganesh's desire to
  keep things tightly scoped.  This is a case where I prefer fewer
  partial functions over tightly controlling access.  NOTE:  I'm not
  changing who has access to the paths, just making the passing of the
  paths explicit.

I think my preferred long term solution would be to roll them into a
global environment and have the commands execute in a special Reader
monad that provides that environment.  How to execute on this long
term goal and the exact details of it need more discussion so I am
explicitly NOT addressing the long term issues of which commands have
access to what.

Let me state my goals one more time:
  * I wanted to reduce the number of partial functions
  * try to make the Annotate source code more readable so that I can
  document it.  I think having fewer exceptional control flow paths
  helps here.

I realize it's a long read, but I wanted to make sure to summarize the
discussion, erring on the side of too much info.

Thanks,
Jason

1 patch for repository http://darcs.net:

Sun Aug 22 03:10:57 PDT 2010  Jason Dagit <dagit@codersbase.com>
  * Remove FixFilePath from DarcsFlag
  By passing the paths explicitly we can remove a few partial functions
  and statically ensure the commands get the paths they need.

Attachments

• remove-fixfilepath-from-darcsflag.dpatch
• unnamed

I forgot to check on the status of my tests before hitting send.  Looks 
like there is a bug in here somewhere.

Jason

This bundle now depends on patch355.


1 patch for repository /Users/dagit/local-data/darcs/darcs.net-HEAD:

Sun Aug 22 05:35:41 PDT 2010  Jason Dagit <dagit@codersbase.com>
  * Remove FixFilePath from DarcsFlag
  By passing the paths explicitly we can remove a few partial functions
  and statically ensure the commands get the paths they need.

Attachments

• remove-fixfilepath-from-darcsflag.dpatch
• unnamed

The amended version passes the tests assuming patch355 gets applied.

I suggest that this doesn't go into 'screened' for now since it's 
relatively controversial and will cause some conflicts.

Alright, I'm going to abuse clarification-requested until we've worked
out what to do with this change

Guillaume, is there any chance you'd be interested in taking a second 
look at this?  It's likely going to need outright rewriting from one of 
us, but since you've done all this work on splitting the Darcs.Repository 
flags from the Darcs.UI flags, maybe you'd have some insight on the 
problem Jason/Ganesh were thinking over?

Here is the patch rewritten for HEAD. I think it makes sense, and
eliminates what seems a useless round trip between packing a couple of
paths as a darcs flag and then checking for their presence (with
functions that can fail).

Attachments

• remove-fixfilepath-from-darcsflag.dpatch

This patch bundle (with 1 patches) was just applied to the repository http://darcs.net/screened.
This message was brought to you by DarcsWatch
http://darcswatch.nomeata.de/repo_http:__darcs.net_screened.html#bundle-1170c69b8ac5101bf35e65bb6729704618d7421d

No regrets, I'm self-accepting the bundle.

This patch bundle (with 1 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-1170c69b8ac5101bf35e65bb6729704618d7421d