[darcs-devel] darcs patch: Add a 'commit' command stub. (and 10 more)

[David Roundy]

On Fri, Jul 27, 2007 at 11:02:33PM +0200, Eric Kow wrote:
> Again, the interesting bit here is the amend-record stuff.
> 
> I hope this time it's more readable and correct, but I wouldn't be surprised if
> there are still mistakes.  It seems quite easy to get these flag interactions
> wrong, so it would be nice to think hard about these, maybe add some test
> cases...

[snip]

> +          actually_get_log p = do logf <- make_log
> +                                  writeBinFile logf $ concat $ intersperse "\n" $ default_log
> +                                  append_info logf p

Shouldn't this be (almost) just

writeBinFile logf $ unlines default_log

The only difference is in the last character.  Would an extra newline hurt
us? I don't think so, as it looks like append_info already checks for the
existence of a trailing newline, and adds one if it's missing.

> +                       let new_author = case get_author opts of
> +                                        Just a  -> a
> +                                        Nothing -> pi_author old_pinf

[...]

> +get_author :: [DarcsFlag] -> Maybe String
> +get_author (Author a:_) = Just a
> +get_author (_:as) = get_author as
> +get_author []     = Nothing

I'm not sure this is quite the semantics we want--although it's not certain
what semantics we do want.  I don't really like the idea of making --author
different from $EMAIL or $DARCS_EMAIL or _darcs/prefs/author.  It's sort of
a surprising behavior.  It'll change the default behavior of darcs if we
change this, but I am thinking that if I amend-record your patch, perhaps
by default darcs ought to change me to the author.  I suppose perhaps
that's less handy if a maintainer wants to amend-record to fix a conflict
or something minor (but wants to maintain attribution), but on the other
hand, it puts the blame for the amend-record in the right place, which is
good.  99% of the time amend-record is only used by the author of the
patch, so it's not a huge deal.  I'd tentatively vote for using
Darcs.Arguments.get_author here.  Perhaps with a modification version that
doesn't prompt the user when it runs out of non-interactive options.

On the whole, it looks good, and I don't see any flag-interaction issues.
-- 
David Roundy
Department of Physics
Oregon State University